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@gmail.com>
---
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.

 xen/common/hypfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c
index acd258edf2..66026ad3e0 100644
--- 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;
     e_len = DIRENTRY_SIZE(e_namelen);
     direntry.e.pad = 0;
     direntry.e.type = template->e.type;
-- 
2.34.1

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@gmail.com>
> ---
> 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

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@gmail.com>
>> ---
>> 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

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@gmail.com>
>>> ---
>>> 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