Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()
On 29.11.19 14:55, Jan Beulich wrote:
On 29.11.2019 14:37, Jürgen Groß wrote:
On 29.11.19 14:26, Jan Beulich wrote:
On 29.11.2019 13:37, Andrew Cooper wrote:
On 29/11/2019 12:19, Jan Beulich wrote:
On 29.11.2019 13:15, Andrew Cooper wrote:
On 29/11/2019 12:13, Jan Beulich wrote:
On 29.11.2019 13:01, Ian Jackson wrote:
Jan Beulich writes ("Re: [PATCH] console: avoid buffer overflow in
guest_console_write()"):
On 29.11.2019 11:22, Andrew Cooper wrote:
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).
I don't know what i wrong with
(size_t)0
which is shorter, even !
True. Yet it contains a cast, no matter how risk-free it may be
in this case. With a cast, I could as well have written (yet
shorter) (size_t)count.
Given that min() has a very strict typecheck, I think we should permit
any use of an explicit cast in a single operand, because it *is* safer
than switching to the min_t() route to make things compile.
Well, I can switch to (size_t)count if this is liked better
overall.
Personally, I'd prefer this option most of all.
Okay, I've switched to this, but while doing so I started wondering
why we'd then not use
kcount = min(count, (unsigned int)sizeof(kbuf) - 1);
which is an (often slightly cheaper) 32-bit operation (and which
is what I had actually started from).
While modifying guest_console_write(), would you mind writing a '\0'
to kbuf[kcount]? There is a "conring_puts(kbuf);" later in this
function which would like a 0 terminated string as input.
That's not the right change for this problem, I think. Now that
we support embedded nul characters, a count should be passed
instead. Julien?
I also wouldn't want to merge this into this patch; I'm happy to
send a separate one.
Yeah, I now realized that it is easy to just add a count parameter to
conring_puts() as it is called only twice and count is already known
at the callsites.
Juergen
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()
On 29/11/2019 13:55, Jan Beulich wrote:
> On 29.11.2019 14:37, Jürgen Groß wrote:
>> On 29.11.19 14:26, Jan Beulich wrote:
>>> On 29.11.2019 13:37, Andrew Cooper wrote:
On 29/11/2019 12:19, Jan Beulich wrote:
> On 29.11.2019 13:15, Andrew Cooper wrote:
>> On 29/11/2019 12:13, Jan Beulich wrote:
>>> On 29.11.2019 13:01, Ian Jackson wrote:
Jan Beulich writes ("Re: [PATCH] console: avoid buffer overflow in
guest_console_write()"):
> On 29.11.2019 11:22, Andrew Cooper wrote:
>> 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).
I don't know what i wrong with
(size_t)0
which is shorter, even !
>>> True. Yet it contains a cast, no matter how risk-free it may be
>>> in this case. With a cast, I could as well have written (yet
>>> shorter) (size_t)count.
>> Given that min() has a very strict typecheck, I think we should permit
>> any use of an explicit cast in a single operand, because it *is* safer
>> than switching to the min_t() route to make things compile.
> Well, I can switch to (size_t)count if this is liked better
> overall.
Personally, I'd prefer this option most of all.
>>> Okay, I've switched to this, but while doing so I started wondering
>>> why we'd then not use
>>>
>>> kcount = min(count, (unsigned int)sizeof(kbuf) - 1);
>>>
>>> which is an (often slightly cheaper) 32-bit operation (and which
>>> is what I had actually started from).
>> While modifying guest_console_write(), would you mind writing a '\0'
>> to kbuf[kcount]? There is a "conring_puts(kbuf);" later in this
>> function which would like a 0 terminated string as input.
> That's not the right change for this problem, I think. Now that
> we support embedded nul characters, a count should be passed
> instead. Julien?
>
> I also wouldn't want to merge this into this patch; I'm happy to
> send a separate one.
I agree. Lets fix the concrete stack corruption issue separately from
the concern over NUL-correctness (which was the purpose of the patch
which introduced this vulnerability to start with).
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()
On 29.11.2019 14:37, Jürgen Groß wrote:
> On 29.11.19 14:26, Jan Beulich wrote:
>> On 29.11.2019 13:37, Andrew Cooper wrote:
>>> On 29/11/2019 12:19, Jan Beulich wrote:
On 29.11.2019 13:15, Andrew Cooper wrote:
> On 29/11/2019 12:13, Jan Beulich wrote:
>> On 29.11.2019 13:01, Ian Jackson wrote:
>>> Jan Beulich writes ("Re: [PATCH] console: avoid buffer overflow in
>>> guest_console_write()"):
On 29.11.2019 11:22, Andrew Cooper wrote:
> 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).
>>> I don't know what i wrong with
>>> (size_t)0
>>> which is shorter, even !
>> True. Yet it contains a cast, no matter how risk-free it may be
>> in this case. With a cast, I could as well have written (yet
>> shorter) (size_t)count.
> Given that min() has a very strict typecheck, I think we should permit
> any use of an explicit cast in a single operand, because it *is* safer
> than switching to the min_t() route to make things compile.
Well, I can switch to (size_t)count if this is liked better
overall.
>>>
>>> Personally, I'd prefer this option most of all.
>>
>> Okay, I've switched to this, but while doing so I started wondering
>> why we'd then not use
>>
>> kcount = min(count, (unsigned int)sizeof(kbuf) - 1);
>>
>> which is an (often slightly cheaper) 32-bit operation (and which
>> is what I had actually started from).
>
> While modifying guest_console_write(), would you mind writing a '\0'
> to kbuf[kcount]? There is a "conring_puts(kbuf);" later in this
> function which would like a 0 terminated string as input.
That's not the right change for this problem, I think. Now that
we support embedded nul characters, a count should be passed
instead. Julien?
I also wouldn't want to merge this into this patch; I'm happy to
send a separate one.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()
On 29.11.19 14:26, Jan Beulich wrote:
On 29.11.2019 13:37, Andrew Cooper wrote:
On 29/11/2019 12:19, Jan Beulich wrote:
On 29.11.2019 13:15, Andrew Cooper wrote:
On 29/11/2019 12:13, Jan Beulich wrote:
On 29.11.2019 13:01, Ian Jackson wrote:
Jan Beulich writes ("Re: [PATCH] console: avoid buffer overflow in
guest_console_write()"):
On 29.11.2019 11:22, Andrew Cooper wrote:
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).
I don't know what i wrong with
(size_t)0
which is shorter, even !
True. Yet it contains a cast, no matter how risk-free it may be
in this case. With a cast, I could as well have written (yet
shorter) (size_t)count.
Given that min() has a very strict typecheck, I think we should permit
any use of an explicit cast in a single operand, because it *is* safer
than switching to the min_t() route to make things compile.
Well, I can switch to (size_t)count if this is liked better
overall.
Personally, I'd prefer this option most of all.
Okay, I've switched to this, but while doing so I started wondering
why we'd then not use
kcount = min(count, (unsigned int)sizeof(kbuf) - 1);
which is an (often slightly cheaper) 32-bit operation (and which
is what I had actually started from).
While modifying guest_console_write(), would you mind writing a '\0'
to kbuf[kcount]? There is a "conring_puts(kbuf);" later in this
function which would like a 0 terminated string as input.
Juergen
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()
On 29.11.2019 13:37, Andrew Cooper wrote:
> On 29/11/2019 12:19, Jan Beulich wrote:
>> On 29.11.2019 13:15, Andrew Cooper wrote:
>>> On 29/11/2019 12:13, Jan Beulich wrote:
On 29.11.2019 13:01, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH] console: avoid buffer overflow in
> guest_console_write()"):
>> On 29.11.2019 11:22, Andrew Cooper wrote:
>>> 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).
> I don't know what i wrong with
>(size_t)0
> which is shorter, even !
True. Yet it contains a cast, no matter how risk-free it may be
in this case. With a cast, I could as well have written (yet
shorter) (size_t)count.
>>> Given that min() has a very strict typecheck, I think we should permit
>>> any use of an explicit cast in a single operand, because it *is* safer
>>> than switching to the min_t() route to make things compile.
>> Well, I can switch to (size_t)count if this is liked better
>> overall.
>
> Personally, I'd prefer this option most of all.
Okay, I've switched to this, but while doing so I started wondering
why we'd then not use
kcount = min(count, (unsigned int)sizeof(kbuf) - 1);
which is an (often slightly cheaper) 32-bit operation (and which
is what I had actually started from).
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()
On 29/11/2019 12:19, Jan Beulich wrote:
> On 29.11.2019 13:15, Andrew Cooper wrote:
>> On 29/11/2019 12:13, Jan Beulich wrote:
>>> On 29.11.2019 13:01, Ian Jackson wrote:
Jan Beulich writes ("Re: [PATCH] console: avoid buffer overflow in
guest_console_write()"):
> On 29.11.2019 11:22, Andrew Cooper wrote:
>> 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).
I don't know what i wrong with
(size_t)0
which is shorter, even !
>>> True. Yet it contains a cast, no matter how risk-free it may be
>>> in this case. With a cast, I could as well have written (yet
>>> shorter) (size_t)count.
>> Given that min() has a very strict typecheck, I think we should permit
>> any use of an explicit cast in a single operand, because it *is* safer
>> than switching to the min_t() route to make things compile.
> Well, I can switch to (size_t)count if this is liked better
> overall.
Personally, I'd prefer this option most of all.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()
On 29.11.2019 13:15, Andrew Cooper wrote:
> On 29/11/2019 12:13, Jan Beulich wrote:
>> On 29.11.2019 13:01, Ian Jackson wrote:
>>> Jan Beulich writes ("Re: [PATCH] console: avoid buffer overflow in
>>> guest_console_write()"):
On 29.11.2019 11:22, Andrew Cooper wrote:
> 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).
>>> I don't know what i wrong with
>>>(size_t)0
>>> which is shorter, even !
>> True. Yet it contains a cast, no matter how risk-free it may be
>> in this case. With a cast, I could as well have written (yet
>> shorter) (size_t)count.
>
> Given that min() has a very strict typecheck, I think we should permit
> any use of an explicit cast in a single operand, because it *is* safer
> than switching to the min_t() route to make things compile.
Well, I can switch to (size_t)count if this is liked better
overall.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()
On 29/11/2019 12:15, Jan Beulich wrote:
> On 29.11.2019 12:59, Ian Jackson wrote:
>> Jan Beulich writes ("[PATCH] console: avoid buffer overflow in
>> guest_console_write()"):
>>> 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
>>> Signed-off-by: Jan Beulich
>> ea601ec9995b included this hunk:
>>
>>case CONSOLEIO_read:
>> +/*
>> + * The return value is either the number of characters read or
>> + * a negative value in case of error. So we need to prevent
>> + * overlap between the two sets.
>> + */
>> +rc = -E2BIG;
>> +if ( count > INT_MAX )
>> +break;
>>
>> Maybe it would be good to move that outside the switch so that it
>> affects CONSOLEIO_write too ?
> And any future subops? And limit output more than necessary (not
> that I think anyone will want to push more than 2G at a time
> through this interface, but anyway)?
Linux is seriously considering initrds > 4G now for various usecases.
2G really isn't enough for everyone, and we shouldn't hardcode blind
presumptions like this.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()
On 29.11.2019 12:59, Ian Jackson wrote:
> Jan Beulich writes ("[PATCH] console: avoid buffer overflow in
> guest_console_write()"):
>> 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
>> Signed-off-by: Jan Beulich
>
> ea601ec9995b included this hunk:
>
>case CONSOLEIO_read:
> +/*
> + * The return value is either the number of characters read or
> + * a negative value in case of error. So we need to prevent
> + * overlap between the two sets.
> + */
> +rc = -E2BIG;
> +if ( count > INT_MAX )
> +break;
>
> Maybe it would be good to move that outside the switch so that it
> affects CONSOLEIO_write too ?
And any future subops? And limit output more than necessary (not
that I think anyone will want to push more than 2G at a time
through this interface, but anyway)?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()
On 29/11/2019 12:13, Jan Beulich wrote:
> On 29.11.2019 13:01, Ian Jackson wrote:
>> Jan Beulich writes ("Re: [PATCH] console: avoid buffer overflow in
>> guest_console_write()"):
>>> On 29.11.2019 11:22, Andrew Cooper wrote:
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).
>> I don't know what i wrong with
>>(size_t)0
>> which is shorter, even !
> True. Yet it contains a cast, no matter how risk-free it may be
> in this case. With a cast, I could as well have written (yet
> shorter) (size_t)count.
Given that min() has a very strict typecheck, I think we should permit
any use of an explicit cast in a single operand, because it *is* safer
than switching to the min_t() route to make things compile.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()
On 29.11.2019 13:01, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH] console: avoid buffer overflow in
> guest_console_write()"):
>> On 29.11.2019 11:22, Andrew Cooper wrote:
>>> 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).
>
> I don't know what i wrong with
>(size_t)0
> which is shorter, even !
True. Yet it contains a cast, no matter how risk-free it may be
in this case. With a cast, I could as well have written (yet
shorter) (size_t)count.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()
On 29/11/2019 12:01, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH] console: avoid buffer overflow in
> guest_console_write()"):
>> On 29.11.2019 11:22, Andrew Cooper wrote:
>>> 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).
> I don't know what i wrong with
>(size_t)0
> which is shorter, even !
Or shorter yet, (size_t)count if you're wanting to go that route.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()
On 29.11.19 11: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
Signed-off-by: Jan Beulich
Release-acked-by: Juergen Gross
Juergen
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()
Jan Beulich writes ("Re: [PATCH] console: avoid buffer overflow in
guest_console_write()"):
> On 29.11.2019 11:22, Andrew Cooper wrote:
> > 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).
I don't know what i wrong with
(size_t)0
which is shorter, even !
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()
Jan Beulich writes ("[PATCH] console: avoid buffer overflow in
guest_console_write()"):
> 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
> Signed-off-by: Jan Beulich
ea601ec9995b included this hunk:
case CONSOLEIO_read:
+/*
+ * The return value is either the number of characters read or
+ * a negative value in case of error. So we need to prevent
+ * overlap between the two sets.
+ */
+rc = -E2BIG;
+if ( count > INT_MAX )
+break;
Maybe it would be good to move that outside the switch so that it
affects CONSOLEIO_write too ?
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()
Hi,
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
Signed-off-by: Jan Beulich
Sorry for the breakage.
Acked-by: Julien Grall
Cheers,
--- 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);
if ( copy_from_guest(kbuf, buffer, kcount) )
return -EFAULT;
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()
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
>> Signed-off-by: Jan Beulich
>>
>> --- 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
> , but ideally using the +0ul form.
Thanks.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()
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
> Signed-off-by: Jan Beulich
>
> --- 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.
That said, given the severity and urgency of this
extremely-lucky-its-not-an-XSA, Reviewed-by: Andrew Cooper
, but ideally using the +0ul form.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
