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