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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?)

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.

Jan