Re: [PATCH-for-5.0 v2] hw/display/sm501: Avoid heap overflow in sm501_2d_operation()

[BALATON Zoltan <balaton@eik.bme.hu>]

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

On Wed, 15 Apr 2020, BALATON Zoltan wrote:
> On Wed, 15 Apr 2020, Peter Maydell wrote:
>> On Mon, 13 Apr 2020 at 23:01, Philippe Mathieu-Daudé <f4bug@amsat.org> 
>> wrote:
>>> 
>>> Zhang Zi Ming reported a heap overflow in the Drawing Engine of
>>> the SM501 companion chip model, in particular in the COPY_AREA()
>>> macro in sm501_2d_operation().
>>> 
>>> Add a simple check to avoid the heap overflow.
>> 
>>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>>> index de0ab9d977..902acb3875 100644
>>> --- a/hw/display/sm501.c
>>> +++ b/hw/display/sm501.c
>>> @@ -726,6 +726,12 @@ static void sm501_2d_operation(SM501State *s)
>>>      int crt = (s->dc_crt_control & SM501_DC_CRT_CONTROL_SEL) ? 1 : 0;
>>>      int fb_len = get_width(s, crt) * get_height(s, crt) * get_bpp(s, 
>>> crt);
>>> 
>>> +    if (rtl && (src_x < operation_width || src_y < operation_height)) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "sm501: Illegal RTL address (%i, 
>>> %i)\n",
>>> +                      src_x, src_y);
>>> +        return;
>>> +    }
>> 
>> This does fix an issue, but I have a feeling that there are
>> other possible guest register value combinations that might
>> cause us to index off one end or the other of the local_mem.
>
> That's what I've meant by it should be reimplemented eventually to fix all 
> possible problems but could not do that before 5.0. Since this is existing 
> bug ever since this device is first committed not patching it now is probably 
> not a big deal if this is not considered a security problem. (And if it is 
> then all the abort() calls are probably a problem too although less serious.)
>
>> The SM501 datasheet is entirely unhelpful on this question, but
>> my suggestion is that we should convert the code so that instead
>> of operating directly on pointers into the middle of the local_mem
>> buffer all the accesses to local_mem go via functions which mask
>> off the high bits of the index. That effectively means that the
>> behaviour if we index off the end of the graphics memory is
>> that we just wrap round to the start of it. It should be fairly
>> easy to be confident that the code isn't accessing off the end
>> of the array and it might even be what the hardware actually does
>> (since it would correspond to 'use low bits of the address to
>> index the ram, ignore high bits')...
>
> Does that make it even slower than it is already? I think it should rather be 
> changed to do what I've done in ati_2d.c and call optimised functions to do 
> the blit operation instead of implementing it directly. Then we'll need

As blits are common operation in several video cards, such as sm501, 
cirrus and ati-vga at least maybe we could also split off some common 
helpers to have one implementation of these which could be secured and 
optimised once and not have to fix it in every device separately. I don't 
volunteer to do that by maybe there's someone who wants to try that?

Regards,
BALATON Zoltan