[Qemu-devel] [PATCH v2] exec: Fix non-power-of-2 sized accesses

[Qemu-devel] [PATCH v2] exec: Fix non-power-of-2 sized accesses

[Alex Williamson]

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;

Re: [Qemu-devel] [PATCH v2] exec: Fix non-power-of-2 sized accesses

[Laszlo Ersek]

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(+)

Re: [Qemu-devel] [PATCH v2] exec: Fix non-power-of-2 sized accesses

[Richard Henderson]

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~

Re: [Qemu-devel] [PATCH v2] exec: Fix non-power-of-2 sized accesses

[Alex Williamson]

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

Re: [Qemu-devel] [PATCH v2] exec: Fix non-power-of-2 sized accesses

[Alex Williamson]

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 :-\

Re: [Qemu-devel] [PATCH v2] exec: Fix non-power-of-2 sized accesses

[Alex Williamson]

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))...

Re: [Qemu-devel] [PATCH v2] exec: Fix non-power-of-2 sized accesses

[Richard Henderson]

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~

Re: [Qemu-devel] [PATCH v2] exec: Fix non-power-of-2 sized accesses

[Eric Blake]

[-- 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 --]