| Message ID | 20200516132352.39E9374594E@zero.eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | ati-vga: Do not allow unaligned access via index register | expand |
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 > >
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 >> >> > >
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 >>> >>> >> >> >
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 >>>> >>>> >>> >>> >> > >
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
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
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
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 */
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(-)