Re: [PATCH] ati-vga: Do not allow unaligned access via index register

[Philippe Mathieu-Daudé]

On 5/17/20 12:40 PM, Philippe Mathieu-Daudé wrote:
> On 5/16/20 5:33 PM, BALATON Zoltan wrote:
> > On Sat, 16 May 2020, Alexander Bulekov wrote:
> > > On 200516 1513, BALATON Zoltan wrote:
> > > > According to docs bits 1 and 0 of MM_INDEX are hard coded to 0 so
> > > > unaligned access via this register should not be possible.
> > > > This also fixes problems reported in bug #1878134.
> > > >
> > > > Signed-off-by: BALATON Zoltan <address@hidden>
> > > > ---
> > >
> > > Hi Zoltan,
> > > I applied this patch and confirmed that I cannot reproduce the crash 
> > > in #1878134
> > > Thanks!
> > >
> > > Acked-by: Alexander Bulekov <address@hidden>
> >
> > Thanks, so that should be Tested-by I think but I don't care much 
> > about tags so whatever works for me.
>
> 'Acked-by' means as a Fuzzer maintainer, Alexander checked your patch 
> and is happy that another maintainer (usually Gerd for hw/display/, as 
> ati.c doesn't have particular maintainer) takes this patch.
>
> You are right, if Alexander tested your patch, he also should add:
> Tested-by: Alexander Bulekov <address@hidden>
>
> If a developer review your patch and agree the logic matches the 
> description and doesn't introduce new regressions, he might reply with a 
> 'Reviewed-by' tag.
>
> Note than tags are not trophies for the patch author, but are helpful 
> for distributions such Debian/Fedora/NetBSD/... when they backport 
> particular patches fixing bugs, before new QEMU (stable) version is 
> released.
>
> Also they are useful in history in case a developer/maintainer goes MIA, 
> there is still others to contact.
>
> Finally, there is a tag documented for bug fixes:
> https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message 
>
>
> If your patch addresses a bug in a public bug tracker, please add a line 
> with "Buglink: <URL-of-the-bug>" there, too.
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1878134
>
> Now, looking at your device implementation, it seems
>
> 1/ The device isn't supposed to have 64-bit accesses
>
> So this might be a more generic fix to Alexander issue:
>
> -- >8 --
> @@ -879,6 +879,7 @@ static void ati_mm_write(void *opaque, hwaddr addr,
>   static const MemoryRegionOps ati_mm_ops = {
>       .read = ati_mm_read,
>       .write = ati_mm_write,
> +    .valid.max_access_size = 4,
>       .endianness = DEVICE_LITTLE_ENDIAN,
>   };
> ---
>
> 2/ All the registers are 32-bit aligned
>
> So you can simplify the implementation by letting 
> access_with_adjusted_size() handle the 8/16-bit accesses by using:
>
> @@ -879,6 +879,8 @@ static void ati_mm_write(void *opaque, hwaddr addr,
>   static const MemoryRegionOps ati_mm_ops = {
>       .read = ati_mm_read,
>       .write = ati_mm_write,
> +    .min.min_access_size = 4,

I meant '.impl.min_access_size'.

>       .endianness = DEVICE_LITTLE_ENDIAN,
>   };
>
> Regards,
>
> Phil.
>
> > Regards,
> > BALATON Zoltan
> >
> > > >  hw/display/ati.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/display/ati.c b/hw/display/ati.c
> > > > index f4c4542751..2ee23173b2 100644
> > > > --- a/hw/display/ati.c
> > > > +++ b/hw/display/ati.c
> > > > @@ -531,7 +531,7 @@ static void ati_mm_write(void *opaque, hwaddr addr,
> > > >      }
> > > >      switch (addr) {
> > > >      case MM_INDEX:
> > > > -        s->regs.mm_index = data;
> > > > +        s->regs.mm_index = data & ~3;
> > > >          break;
> > > >      case MM_DATA ... MM_DATA + 3:
> > > >          /* indexed access to regs or memory */
> > > > --
> > > > 2.21.3