| Message ID | 20220724173159.410535-1-burzalodowa@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/hypfs: check the return value of snprintf to avoid leaking stack accidently | expand |
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.
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
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;
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(+)