Re: [PATCH] xen/hypfs: check the return value of snprintf to avoid leaking stack accidently

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

On 7/25/22 11:00, Jan Beulich wrote:
> On 24.07.2022 19:31, Xenia Ragiadakou wrote:
> > The function snprintf() returns the number of characters that would have been
> > written in the buffer if the buffer size had been sufficiently large,
> > not counting the terminating null character.
> > Hence, the value returned is not guaranteed to be smaller than the buffer size.
> > Check the return value of snprintf to prevent leaking stack contents to the
> > guest by accident.
> >
> > Signed-off-by: Xenia Ragiadakou <burzalodowa@xxxxxxxxx>
> > ---
> > I 've noticed that in general in xen the return value of snprintf is not
> > checked. Is there a particular reason for this? I mean if there is no space to
> > fit the entire string, is it preferable to write only a part of it instead of
> > failing? If that's the case, then scnprintf could be used instead below.
> You will find lack of checking particularly in cases where the buffer size
> has been chosen to always fit the (expected) to-be-formatted value(s).
> While in a number of (most?) cases this ends up being fragile when
> considering general portability (like assuming that "unsigned int" can
> always be expressed in 10 decimal digits), I guess making such assumptions
> has been deemed "good enough" up until now. I think this also applies here,
> so ...
>
> > --- a/xen/common/hypfs.c
> > +++ b/xen/common/hypfs.c
> > @@ -377,6 +377,8 @@ int hypfs_read_dyndir_id_entry(const struct hypfs_entry_dir 
> > *template,
> >       unsigned int e_namelen, e_len;
> >   
> >       e_namelen = snprintf(name, sizeof(name), template->e.name, id);
> > +    if ( e_namelen >= sizeof(name) )
> > +        return -ENOBUFS;
> ... I wonder whether you don't want to additionally put ASSERT_UNREACHABLE()
> here (but leave -ENOBUFS to keep release builds safe). (I also take it that
> you haven't found an actual case of the buffer being too small here?)
hypfs_read_dyndir_id_entry() currently is used only by the cpupool 
pooldir and the name needs to hold an unsigned int. So, currently there 
is not a case of the buffer being too small.
> But of course the main purpose of using snprintf() is to avoid buffer
> overrun, so truncation is indeed deemed only secondary in many cases.
> Which doesn't mean adding such checks would be unwelcome - it's just that
> in some of the cases your options are limited - see e.g. the other similar
> use of snprintf() in hypfs_gen_dyndir_id_entry(), where the function doesn't
> presently have any error cases.
What I considered an issue here is that when copying the buffer to the 
guest we use the value returned by snprintf().
copy_to_guest_offset(*uaddr, DIRENTRY_NAME_OFF, name, e_namelen + 1)

Also, if truncation is not considered an issue, I can remove the check 
and replace snprintf with scnprintf.
--
Xenia