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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.07.2022 10:22, Xenia Ragiadakou wrote:
> 
> 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.

Well, it is my view that truncation (if any could happen here) is an
issue which does not want papering over. So I agree with you sticking
to snprintf(). Still I think there's an internal assumption that
truncation should not happen here, hence I've suggested the addition
of an assertion to this effect.

Jan