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

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

[BALATON Zoltan]

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

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

[Alexander Bulekov]

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>

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

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

[BALATON Zoltan]

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.

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

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

[Philippe Mathieu-Daudé]

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

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

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

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2530 bytes --]

On Sun, 17 May 2020, Philippe Mathieu-Daudé wrote:
> 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:
>> 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

Does this reply add that tag already or do I need to submit a v2 with it 
(or the maintainer could add it when merging)?

>> 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,
>>   };
>> ---

I've tried that first but it does not work. The reason is that ati_mm_read 
is recursively called for indexed access via MM_DATA which causes the 
problem that happens when MM_INDEX is set to a non-aligned value. No 64 
bit access, just 32 bit with offset of 2 bytes as can be seen from the 
stach trace I've attached to the bug. Fortunately indexed access is 
documented to only support aligned access by not allowing setting low bits 
of MM_INDEX so unless we find a client needing it my patch should do it.

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

I think this would not work either because not all registers are the same, 
some only can be read all 32 bits, some also 16 or 8 bits and clients do 
access these with less than 32 bits and accessing parts of the reg may 
trigger actions so the current way is probably better and necessary to 
correctly support different valid and invalid unaligned accessses.

Regards,
BALATON Zoltan

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

[Philippe Mathieu-Daudé]

On 5/17/20 4:30 PM, BALATON Zoltan wrote:
> On Sun, 17 May 2020, Philippe Mathieu-Daudé wrote:
>> 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:
>>> 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
> 
> Does this reply add that tag already or do I need to submit a v2 with it 
> (or the maintainer could add it when merging)?

If he doesn't have time he can reply to your patch :)

> 
>>> 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,
>>>   };
>>> ---
> 
> I've tried that first but it does not work. The reason is that 
> ati_mm_read is recursively called for indexed access via MM_DATA which 
> causes the problem that happens when MM_INDEX is set to a non-aligned 
> value. No 64 bit access, just 32 bit with offset of 2 bytes as can be 
> seen from the stach trace I've attached to the bug. Fortunately indexed 
> access is documented to only support aligned access by not allowing 
> setting low bits of MM_INDEX so unless we find a client needing it my 
> patch should do it.

OK, so this is another device affected by the memory.c lacking of 
unaligned access (Gerd saw another one with USB).

> 
>>> 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'.
> 
> I think this would not work either because not all registers are the 
> same, some only can be read all 32 bits, some also 16 or 8 bits and 
> clients do access these with less than 32 bits and accessing parts of 
> the reg may trigger actions so the current way is probably better and 
> necessary to correctly support different valid and invalid unaligned 
> accessses.

.valid.xxx_access_size is what the guest are allowed to use,
.impl.xxx_access_size is what the developer had in mind when writing the 
model.

.impl.min_access_size = 4 doesn't forbid 8/16-bit guest accesses.

Moreover, it overloads you the burden of handling short accesses.

Anyway this was just a suggested simplification.

> 
> Regards,
> BALATON Zoltan

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

[Gerd Hoffmann]

  Hi,

> > > Buglink: https://bugs.launchpad.net/qemu/+bug/1878134
> 
> Does this reply add that tag already or do I need to submit a v2 with it (or
> the maintainer could add it when merging)?

Patch added to vga queue.

Buglink added too.  Doesn't happen automatically though so having this
in the initial post right from the start makes things easier for me.

thanks,
  Gerd