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

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

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 <balaton@eik.bme.hu>
>>> ---
>>
>> Hi Zoltan,
>> I applied this patch and confirmed that I cannot reproduce the crash 
>> in #1878134
>> Thanks!
>>
>> Acked-by: Alexander Bulekov <alxndr@bu.edu>
> 
> 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 <alxndr@bu.edu>

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