Re: [PATCH] ui/cursor: incomplete check for integer overflow in cursor_alloc

[Mauro Matteo Cascella]

On Tue, May 23, 2023 at 4:07 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 23/5/23 14:57, Mauro Matteo Cascella wrote:
> > On Tue, May 23, 2023 at 10:37 AM Philippe Mathieu-Daudé
> > <philmd@linaro.org> wrote:
> >>
> >> On 23/5/23 10:09, Daniel P. Berrangé wrote:
> >>> On Mon, May 22, 2023 at 08:55:02PM +0200, Philippe Mathieu-Daudé wrote:
> >>>> On 9/5/23 09:13, Marc-André Lureau wrote:
> >>>>> Hi
> >>>>>
> >>>>> On Mon, May 8, 2023 at 6:21 PM Mauro Matteo Cascella
> >>>>> <mcascell@redhat.com <mailto:mcascell@redhat.com>> wrote:
> >>>>>
> >>>>>       The cursor_alloc function still accepts a signed integer for both
> >>>>>       the cursor
> >>>>>       width and height. A specially crafted negative width/height could
> >>>>>       make datasize
> >>>>>       wrap around and cause the next allocation to be 0, potentially
> >>>>>       leading to a
> >>>>>       heap buffer overflow. Modify QEMUCursor struct and cursor_alloc
> >>>>>       prototype to
> >>>>>       accept unsigned ints.
> >>>>>
> >>>>>       Fixes: CVE-2023-1601
> >>>>>       Fixes: fa892e9a ("ui/cursor: fix integer overflow in cursor_alloc
> >>>>>       (CVE-2021-4206)")
> >>>>>       Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com
> >>>>>       <mailto:mcascell@redhat.com>>
> >>>>>       Reported-by: Jacek Halon <jacek.halon@gmail.com
> >>>>>       <mailto:jacek.halon@gmail.com>>
> >>>>>
> >>>>>
> >>>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com
> >>>>> <mailto:marcandre.lureau@redhat.com>>
> >>>>>
> >>>>> It looks like this is not exploitable, QXL code uses u16 types, and
> >>>>
> >>>> 0xffff * 0xffff * 4 still overflows on 32-bit host, right?
> >>>
> >>> cursor_alloc() will reject 0xffff:
> >>>
> >>>       if (width > 512 || height > 512) {
> >>>           return NULL;
> >>>       }
> >>
> >> I hadn't looked at the source file (the 'datasize' assignation
> >> made me incorrectly think it'd be use before sanitized).
> >>
> >> Still I wonder why can't we use a simple 'unsigned' type instead
> >> of a uint32_t, but I won't insist.
> >
> > I can send v2 with s/uint32_t/uint16_t/ if you think it's a relevant change.
>
> Specifying the word size doesn't really add any (security) value IMHO.

No security benefit, I know, it just seems more reasonable given what
Gerd said about 512x512 sprites.

> I'll stop bikeshedding here.
>
> Regards,
>
> Phil.
>
--
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0