exec: Fix non-power-of-2 sized accesses

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

In the example I saw the guest was doing a 4-byte read at I/O port
0xcd7.  We satisfy the first byte with a 1-byte read leaving 3 bytes
remaining at an 8-byte aligned address... boom.  ffs() caused weird
stack smashing errors here, so I just did a loop since it can only
run for a few iterations max.

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

On 08/16/13 06:55, 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
> ---
> 
> In the example I saw the guest was doing a 4-byte read at I/O port
> 0xcd7.  We satisfy the first byte with a 1-byte read leaving 3 bytes
> remaining at an 8-byte aligned address... boom.  ffs() caused weird
> stack smashing errors here, so I just did a loop since it can only
> run for a few iterations max.
> 
>  exec.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/exec.c b/exec.c
> index 3ca9381..652fc3a 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) {
> +            access_size_max >>= 1;
> +        }
> +    }
> +
>      /* Don't attempt accesses larger than the maximum.  */
>      if (l > access_size_max) {
>          l = access_size_max;
> 
> 

Assuming that "access_size_max" is positive when reaching the code
you're adding (and it does seem positive at that point), you don't need
"&& access_size_max > 1". That expression won't be evaluated when it
would matter (ie. when access_size_max==1).

Anyway that's not a bug.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

On Fri, 2013-08-16 at 09:10 +0200, Laszlo Ersek wrote:
> On 08/16/13 06:55, 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
> > ---
> > 
> > In the example I saw the guest was doing a 4-byte read at I/O port
> > 0xcd7.  We satisfy the first byte with a 1-byte read leaving 3 bytes
> > remaining at an 8-byte aligned address... boom.  ffs() caused weird
> > stack smashing errors here, so I just did a loop since it can only
> > run for a few iterations max.
> > 
> >  exec.c |    7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/exec.c b/exec.c
> > index 3ca9381..652fc3a 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) {
> > +            access_size_max >>= 1;
> > +        }
> > +    }
> > +
> >      /* Don't attempt accesses larger than the maximum.  */
> >      if (l > access_size_max) {
> >          l = access_size_max;
> > 
> > 
> 
> Assuming that "access_size_max" is positive when reaching the code
> you're adding (and it does seem positive at that point), you don't need
> "&& access_size_max > 1". That expression won't be evaluated when it
> would matter (ie. when access_size_max==1).
> 
> Anyway that's not a bug.
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

I realized this after I went to bed too.  I'll send a v2 w/o the second
condition.  Thanks,

Alex