On 23/5/23 14:57, Mauro Matteo Cascella wrote:
On Tue, May 23, 2023 at 10:37 AM Philippe Mathieu-Daudé
<phi...@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
<mcasc...@redhat.com <mailto:mcasc...@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 <mcasc...@redhat.com
<mailto:mcasc...@redhat.com>>
Reported-by: Jacek Halon <jacek.ha...@gmail.com
<mailto:jacek.ha...@gmail.com>>
Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com
<mailto:marcandre.lur...@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.
