Re: [PATCH v3 7/8] hw/display/artist: Refactor artist_rop8() to avoid buffer over-run

[Helge Deller <deller@gmx.de>]

On 09.08.20 19:17, Alexander Bulekov wrote:
> On 200809 0717, Helge Deller wrote:
>> Hello Alexander,
>>
>> On 06.08.20 17:46, Alexander Bulekov wrote:
>>> On 200805 2244, Helge Deller wrote:
>>>> * Alexander Bulekov <alxndr@bu.edu>:
>>>>> On 200804 2320, Helge Deller wrote:
>>>>>> * Alexander Bulekov <alxndr@bu.edu>:
>>>>>>> I applied this series and it fixes most of the problems I saw before.
>>>>>>> I still see a few crashes - I made issues for them on launchpad:
>>>>>>> https://bugs.launchpad.net/qemu/+bug/1890310
>>>>>>> https://bugs.launchpad.net/qemu/+bug/1890311
>>>>>>> https://bugs.launchpad.net/qemu/+bug/1890312
>>>>> Hi Helge, I can still reproduce this one  ^^^
>>>>> I'll fuzz it some more, but so far I am not finding anything else.
>>>>
>>>> I've now updated the patch which does address all issues you found
>>>> so far. It's attached below.
>>>>
>>> I pulled from your repo, but I can still reproduce LP1890312
>>> (I build with --enable-sanitizers). Maybe I am missing something? With
>>> this cleared up, I'm happy to leave an Acked-by for the artist patches
>>> in this series.
>>>
>>> git show --summary
>>> commit 1657a7a95adc15552138c2b4d310a06128093892 (HEAD, hppa/target-hppa, target-hppa)
>>> Author: Helge Deller <deller@gmx.de>
>>> Date:   Tue Aug 4 15:35:38 2020 +0200
>>
>> The current tree at
>> https://github.com/hdeller/qemu-hppa/commits/target-hppa
>> does survive all your tests and in addition fixes an artist bug which
>> made the dtwm window manager rendering wrong on HP-UX.
>> Please completely revert your tree before pulling again.
>> I'll send out a complete patch series shortly.
>>
>
> Hi Helge,
> This still causes a segfault for me:
>
> cat << EOF | ./hppa-softmmu/qemu-system-hppa -m 64 -display none \
> -qtest stdio -accel qtest
> writew 0xf8118001 0x105a
> readq 0xf900f8ff
> EOF

It's strange, I don't know why, but I can't reproduce it:
[deller@ls3530 qemu-helge-system]$ cat << EOF | ./hppa-softmmu/qemu-system-hppa -m 64 -display none -qtest stdio -accel qtest
writew 0xf8118001 0x105a
readq 0xf900f8ff
EOF
[I 1596999589.838131] OPENED
[R +0.006113] writew 0xf8118001 0x105a
OK
[S +0.006128] OK
[R +0.006137] readq 0xf900f8ff
OK 0x0000000000000000
[S +0.006141] OK 0x0000000000000000
[I +0.006163] CLOSED


Nevertheless, you are correct that there is a bounds check missing!

> I think the problem is that there is a bounds check missing on the
> artist_vram_read with s->cmap_bm_access path. The bounds check is
> present for artist_vram_write, so I just copied it over, and that fixed
> the issue (applied on top of the current qemu-hppa/target-hppa):

I'll apply it with minor modifications...

>
> diff --git a/hw/display/artist.c b/hw/display/artist.c
> index 720db179ab..678023bb3a 100644
> --- a/hw/display/artist.c
> +++ b/hw/display/artist.c
> @@ -1216,8 +1216,10 @@ static uint64_t artist_vram_read(void *opaque, hwaddr addr, unsigned size)
>
>      if (s->cmap_bm_access) {
>          buf = &s->vram_buffer[ARTIST_BUFFER_CMAP];
> -        val = *(uint32_t *)(buf->data + addr);
> -        trace_artist_vram_read(size, addr, 0, 0, val);
> +        if (addr + 3 < buf->size) {

We need to check addr < buf->size too, otherwise with addr = 0xffffffff would succeed too:
	    if (addr < buf->size && addr + 3 < buf->size) {

> +            val = *(uint32_t *)(buf->data + addr);
> +            trace_artist_vram_read(size, addr, 0, 0, val);
> +        }
>          return 0;

This seems wrong... should be "return val;", otherwise the whole function
doesn't make sense.

Thank you!
Helge