| Message ID | 5766dd2b-2aa7-bafe-56ad-3ea33ddf4591@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | console: avoid buffer overflow in guest_console_write() | expand |
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@ioactive.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- 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 <andrew.cooper3@citrix.com>, but ideally using the +0ul form. ~Andrew
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@ioactive.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- 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@citrix.com>, but ideally using the +0ul form. Thanks. Jan
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 <ivansprundel@ioactive.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> Sorry for the breakage. Acked-by: Julien Grall <julien@xen.org> 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; > >
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 <ivansprundel@ioactive.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
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.
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.
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 <ivansprundel@ioactive.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> Release-acked-by: Juergen Gross <jgross@suse.com> Juergen
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
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
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
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 <ivansprundel@ioactive.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > 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
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 <ivansprundel@ioactive.com> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> 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
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
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
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
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
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
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
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
--- 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;
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@ioactive.com> Signed-off-by: Jan Beulich <jbeulich@suse.com>