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 <[email protected]> >>> --- >>> 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
