All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2] exec: Fix non-power-of-2 sized accesses
@ 2013-08-16 12:50 Alex Williamson
  2013-08-16 13:37 ` Laszlo Ersek
  2013-08-16 15:27 ` Richard Henderson
  0 siblings, 2 replies; 8+ messages in thread
From: Alex Williamson @ 2013-08-16 12:50 UTC (permalink / raw)
  To: qemu-devel; +Cc: lersek, qemu-stable, rth

Since commit 23326164 we align access sizes to match the alignment of
the address, but we don't align the access size itself.  This means we
let illegal access sizes (ex. 3) slip through if the address is
sufficiently aligned (ex. 4).  This results in an abort which would be
easy for a guest to trigger.  Account for aligning the access size.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Cc: qemu-stable@nongnu.org
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---

v2: Remove unnecessary loop condition

 exec.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/exec.c b/exec.c
index 3ca9381..3c19147 100644
--- a/exec.c
+++ b/exec.c
@@ -1924,6 +1924,13 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
         }
     }
 
+    /* Size must be a power of 2 */
+    if (l & (l - 1)) {
+        while (l & (access_size_max - 1)) {
+            access_size_max >>= 1;
+        }
+    }
+
     /* Don't attempt accesses larger than the maximum.  */
     if (l > access_size_max) {
         l = access_size_max;

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v2] exec: Fix non-power-of-2 sized accesses
  2013-08-16 12:50 [Qemu-devel] [PATCH v2] exec: Fix non-power-of-2 sized accesses Alex Williamson
@ 2013-08-16 13:37 ` Laszlo Ersek
  2013-08-16 15:27 ` Richard Henderson
  1 sibling, 0 replies; 8+ messages in thread
From: Laszlo Ersek @ 2013-08-16 13:37 UTC (permalink / raw)
  To: Alex Williamson; +Cc: rth, qemu-devel, qemu-stable

On 08/16/13 14:50, Alex Williamson wrote:
> Since commit 23326164 we align access sizes to match the alignment of
> the address, but we don't align the access size itself.  This means we
> let illegal access sizes (ex. 3) slip through if the address is
> sufficiently aligned (ex. 4).  This results in an abort which would be
> easy for a guest to trigger.  Account for aligning the access size.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> Cc: qemu-stable@nongnu.org
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

yeah

> ---
> 
> v2: Remove unnecessary loop condition
> 
>  exec.c |    7 +++++++
>  1 file changed, 7 insertions(+)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v2] exec: Fix non-power-of-2 sized accesses
  2013-08-16 12:50 [Qemu-devel] [PATCH v2] exec: Fix non-power-of-2 sized accesses Alex Williamson
  2013-08-16 13:37 ` Laszlo Ersek
@ 2013-08-16 15:27 ` Richard Henderson
  2013-08-16 15:37   ` Alex Williamson
  1 sibling, 1 reply; 8+ messages in thread
From: Richard Henderson @ 2013-08-16 15:27 UTC (permalink / raw)
  To: Alex Williamson; +Cc: lersek, qemu-devel, qemu-stable

On 08/16/2013 05:50 AM, Alex Williamson wrote:
> +    /* Size must be a power of 2 */
> +    if (l & (l - 1)) {
> +        while (l & (access_size_max - 1)) {
> +            access_size_max >>= 1;
> +        }
> +    }
> +

Why the loop at all?  x & -x extracts the lsb of x.


r~

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v2] exec: Fix non-power-of-2 sized accesses
  2013-08-16 15:27 ` Richard Henderson
@ 2013-08-16 15:37   ` Alex Williamson
  2013-08-16 15:43     ` Alex Williamson
  2013-08-16 19:46     ` Richard Henderson
  0 siblings, 2 replies; 8+ messages in thread
From: Alex Williamson @ 2013-08-16 15:37 UTC (permalink / raw)
  To: Richard Henderson; +Cc: lersek, qemu-devel, qemu-stable

On Fri, 2013-08-16 at 08:27 -0700, Richard Henderson wrote:
> On 08/16/2013 05:50 AM, Alex Williamson wrote:
> > +    /* Size must be a power of 2 */
> > +    if (l & (l - 1)) {
> > +        while (l & (access_size_max - 1)) {
> > +            access_size_max >>= 1;
> > +        }
> > +    }
> > +
> 
> Why the loop at all?  x & -x extracts the lsb of x.

l = 5, we want 4, not 1.  Thanks,

Alex

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v2] exec: Fix non-power-of-2 sized accesses
  2013-08-16 15:37   ` Alex Williamson
@ 2013-08-16 15:43     ` Alex Williamson
  2013-08-16 15:46       ` Alex Williamson
  2013-08-16 19:46     ` Richard Henderson
  1 sibling, 1 reply; 8+ messages in thread
From: Alex Williamson @ 2013-08-16 15:43 UTC (permalink / raw)
  To: Richard Henderson; +Cc: lersek, qemu-devel, qemu-stable

On Fri, 2013-08-16 at 09:37 -0600, Alex Williamson wrote:
> On Fri, 2013-08-16 at 08:27 -0700, Richard Henderson wrote:
> > On 08/16/2013 05:50 AM, Alex Williamson wrote:
> > > +    /* Size must be a power of 2 */
> > > +    if (l & (l - 1)) {
> > > +        while (l & (access_size_max - 1)) {
> > > +            access_size_max >>= 1;
> > > +        }
> > > +    }
> > > +
> > 
> > Why the loop at all?  x & -x extracts the lsb of x.
> 
> l = 5, we want 4, not 1.  Thanks,

Which is not what my patch does either :-\

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v2] exec: Fix non-power-of-2 sized accesses
  2013-08-16 15:43     ` Alex Williamson
@ 2013-08-16 15:46       ` Alex Williamson
  0 siblings, 0 replies; 8+ messages in thread
From: Alex Williamson @ 2013-08-16 15:46 UTC (permalink / raw)
  To: Richard Henderson; +Cc: lersek, qemu-devel, qemu-stable

On Fri, 2013-08-16 at 09:43 -0600, Alex Williamson wrote:
> On Fri, 2013-08-16 at 09:37 -0600, Alex Williamson wrote:
> > On Fri, 2013-08-16 at 08:27 -0700, Richard Henderson wrote:
> > > On 08/16/2013 05:50 AM, Alex Williamson wrote:
> > > > +    /* Size must be a power of 2 */
> > > > +    if (l & (l - 1)) {
> > > > +        while (l & (access_size_max - 1)) {
> > > > +            access_size_max >>= 1;
> > > > +        }
> > > > +    }
> > > > +
> > > 
> > > Why the loop at all?  x & -x extracts the lsb of x.
> > 
> > l = 5, we want 4, not 1.  Thanks,
> 
> Which is not what my patch does either :-\

Simple change though

  while (!(l & access_size_max))...

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v2] exec: Fix non-power-of-2 sized accesses
  2013-08-16 15:37   ` Alex Williamson
  2013-08-16 15:43     ` Alex Williamson
@ 2013-08-16 19:46     ` Richard Henderson
  2013-08-16 20:04       ` Eric Blake
  1 sibling, 1 reply; 8+ messages in thread
From: Richard Henderson @ 2013-08-16 19:46 UTC (permalink / raw)
  To: Alex Williamson; +Cc: lersek, qemu-devel, qemu-stable

On 08/16/2013 08:37 AM, Alex Williamson wrote:
> On Fri, 2013-08-16 at 08:27 -0700, Richard Henderson wrote:
>> On 08/16/2013 05:50 AM, Alex Williamson wrote:
>>> +    /* Size must be a power of 2 */
>>> +    if (l & (l - 1)) {
>>> +        while (l & (access_size_max - 1)) {
>>> +            access_size_max >>= 1;
>>> +        }
>>> +    }
>>> +
>>
>> Why the loop at all?  x & -x extracts the lsb of x.
> 
> l = 5, we want 4, not 1.  Thanks,

It still doesn't require a loop.

  l = 1 << (31 - clz32(l));


r~

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v2] exec: Fix non-power-of-2 sized accesses
  2013-08-16 19:46     ` Richard Henderson
@ 2013-08-16 20:04       ` Eric Blake
  0 siblings, 0 replies; 8+ messages in thread
From: Eric Blake @ 2013-08-16 20:04 UTC (permalink / raw)
  To: Richard Henderson; +Cc: Alex Williamson, lersek, qemu-devel, qemu-stable

[-- Attachment #1: Type: text/plain, Size: 832 bytes --]

On 08/16/2013 01:46 PM, Richard Henderson wrote:
> On 08/16/2013 08:37 AM, Alex Williamson wrote:
>> On Fri, 2013-08-16 at 08:27 -0700, Richard Henderson wrote:
>>> On 08/16/2013 05:50 AM, Alex Williamson wrote:
>>>> +    /* Size must be a power of 2 */
>>>> +    if (l & (l - 1)) {
>>>> +        while (l & (access_size_max - 1)) {
>>>> +            access_size_max >>= 1;
>>>> +        }
>>>> +    }
>>>> +
>>>
>>> Why the loop at all?  x & -x extracts the lsb of x.
>>
>> l = 5, we want 4, not 1.  Thanks,
> 
> It still doesn't require a loop.
> 
>   l = 1 << (31 - clz32(l));

or even pow2floor() declared in qemu-common.h and implemented in
cutils.c.  No need to reinvent what we already have.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2013-08-16 20:04 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-16 12:50 [Qemu-devel] [PATCH v2] exec: Fix non-power-of-2 sized accesses Alex Williamson
2013-08-16 13:37 ` Laszlo Ersek
2013-08-16 15:27 ` Richard Henderson
2013-08-16 15:37   ` Alex Williamson
2013-08-16 15:43     ` Alex Williamson
2013-08-16 15:46       ` Alex Williamson
2013-08-16 19:46     ` Richard Henderson
2013-08-16 20:04       ` Eric Blake

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.