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

[Qemu-devel] [PATCH v3] 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
---

v3: Highest power of 2, not lowest
v2: Remove unnecessary loop condition

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

diff --git a/exec.c b/exec.c
index 3ca9381..8c90cef 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) && 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 v3] exec: Fix non-power-of-2 sized accesses

[Laszlo Ersek]

On 08/16/13 18:00, 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
> ---
> 
> v3: Highest power of 2, not lowest
> v2: Remove unnecessary loop condition
> 
>  exec.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/exec.c b/exec.c
> index 3ca9381..8c90cef 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) && 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;
> 

Apologies, but I'm now totally confused.

Suppose that the new code is reached with (access_size_max == 4).

Now, l==9 and l==3 will enter the loop just the same, both shifting
"access_size_max" right at least once, even though 9 is greater than 4,
and 3 is less than 4.

Is that OK? What's the goal here?

Sorry for being dense...

Thanks,
Laszlo

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

[Alex Williamson]

On Fri, 2013-08-16 at 23:00 +0200, Laszlo Ersek wrote:
> On 08/16/13 18:00, 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
> > ---
> > 
> > v3: Highest power of 2, not lowest
> > v2: Remove unnecessary loop condition
> > 
> >  exec.c |    7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/exec.c b/exec.c
> > index 3ca9381..8c90cef 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) && 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;
> > 
> 
> Apologies, but I'm now totally confused.
> 
> Suppose that the new code is reached with (access_size_max == 4).
> 
> Now, l==9 and l==3 will enter the loop just the same, both shifting
> "access_size_max" right at least once, even though 9 is greater than 4,
> and 3 is less than 4.

*sigh*, just as I was getting ready to point out that the above is
faster than pow2floor, you have to go and point out that the result is
wrong ;)  I don't think clz or pow2floor is the answer though, the
problem size is too small.  Rather than trying to solve this with an
algorithm, I think we just need a simple:

if (size >= 8)
	size = 8;
else if (size >=4)
	size = 4;
...

We only have 4 cases to deal with and it comes out a couple times faster
than pow2floor.  v4...  Thanks,

Alex