Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.11.2019 11:22, Andrew Cooper wrote:
> On 29/11/2019 10:13, Jan Beulich wrote:
>> The switch of guest_console_write()'s second parameter from plain to
>> unsigned int has caused the function's main loop header to no longer
>> guard the min_t() use within the function against effectively negative
>> values, due to the casts hidden inside the macro. Replace by a plain
>> min(), converting one of the arguments suitably without involving any
>> cast.
>>
>> Fixes: ea601ec9995b ("xen/console: Rework HYPERCALL_console_io interface")
>> Reported-by: Ilja Van Sprundel <ivansprundel@xxxxxxxxxxxx>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> --- a/xen/drivers/char/console.c
>> +++ b/xen/drivers/char/console.c
>> @@ -538,7 +538,7 @@ static long guest_console_write(XEN_GUES
>>                  __HYPERVISOR_console_io, "iih",
>>                  CONSOLEIO_write, count, buffer);
>>  
>> -        kcount = min_t(int, count, sizeof(kbuf)-1);
>> +        kcount = min(count + sizeof(char[0]), sizeof(kbuf) - 1);
> 
> Is sizeof(array[0]) always 0, or is this just a GCC-ism ?  Godbolt
> suggests is 0 on all compiler we support.
> 
> Either way, isn't the more common idiom + 0ul ?  Personally, I feel that
> is clearer to follow.

I decided against + 0ul or alike because in principle size_t
and unsigned long are different types. In particular 32-bit
x86 gcc uses unsigned int for size_t, and hence min()'s
type safety check would cause the build to fail there. The
same risk obviously exists for any 32-bit arch (e.g. Arm32,
but I haven't checked what type it actually uses).

> That said, given the severity and urgency of this
> extremely-lucky-its-not-an-XSA, Reviewed-by: Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>, but ideally using the +0ul form.

Thanks.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel