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

[Philippe Mathieu-Daudé]

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.

I'll stop bikeshedding here.

Regards,

Phil.