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

[Jürgen Groß <jgross@xxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel