| Message ID | 20220804124736.1827246-1-burzalodowa@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] xen/hypfs: check the return value of snprintf to avoid leaking stack accidently | expand |
On 04.08.2022 14:47, Xenia Ragiadakou wrote: > Changes in v2: > - add ASSERT_UNREACHABLE() Hmm, this ... > --- a/xen/common/hypfs.c > +++ b/xen/common/hypfs.c > @@ -377,8 +377,10 @@ 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) ) > + if ( e_namelen >= sizeof(name) ) { > + ASSERT_UNREACHABLE(); > return -ENOBUFS; > + } ... looks to be an incremental patch on top of v1, not v2 of that patch? Also please correct the placement of the opening brace. Jan
On 04.08.22 14:47, 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. > > Also, for debug builds, add an assertion to ensure that the assumption made on > the size of the destination buffer still holds. > > Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com> Reviewed-by: Juergen Gross <jgross@suse.com> Juergen
Hi Jan, On 8/4/22 16:01, Jan Beulich wrote: > On 04.08.2022 14:47, Xenia Ragiadakou wrote: >> Changes in v2: >> - add ASSERT_UNREACHABLE() > > Hmm, this ... > >> --- a/xen/common/hypfs.c >> +++ b/xen/common/hypfs.c >> @@ -377,8 +377,10 @@ 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) ) >> + if ( e_namelen >= sizeof(name) ) { >> + ASSERT_UNREACHABLE(); >> return -ENOBUFS; >> + } > > ... looks to be an incremental patch on top of v1, not v2 of that > patch? So, here, IIUC, I have to create a patch series and add the assert in the second (2/2) patch? What should be the version number of the series? > Also please correct the placement of the opening brace. Ah, ok. Sorry.
On 04.08.2022 15:10, Xenia Ragiadakou wrote: > On 8/4/22 16:01, Jan Beulich wrote: >> On 04.08.2022 14:47, Xenia Ragiadakou wrote: >>> Changes in v2: >>> - add ASSERT_UNREACHABLE() >> >> Hmm, this ... >> >>> --- a/xen/common/hypfs.c >>> +++ b/xen/common/hypfs.c >>> @@ -377,8 +377,10 @@ 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) ) >>> + if ( e_namelen >= sizeof(name) ) { >>> + ASSERT_UNREACHABLE(); >>> return -ENOBUFS; >>> + } >> >> ... looks to be an incremental patch on top of v1, not v2 of that >> patch? > > So, here, IIUC, I have to create a patch series and add the assert in > the second (2/2) patch? What should be the version number of the series? No, why? Simply fold this change into the earlier one, and call the result v3. Jan
On 8/4/22 16:13, Jan Beulich wrote: > On 04.08.2022 15:10, Xenia Ragiadakou wrote: >> On 8/4/22 16:01, Jan Beulich wrote: >>> On 04.08.2022 14:47, Xenia Ragiadakou wrote: >>>> Changes in v2: >>>> - add ASSERT_UNREACHABLE() >>> >>> Hmm, this ... >>> >>>> --- a/xen/common/hypfs.c >>>> +++ b/xen/common/hypfs.c >>>> @@ -377,8 +377,10 @@ 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) ) >>>> + if ( e_namelen >= sizeof(name) ) { >>>> + ASSERT_UNREACHABLE(); >>>> return -ENOBUFS; >>>> + } >>> >>> ... looks to be an incremental patch on top of v1, not v2 of that >>> patch? >> >> So, here, IIUC, I have to create a patch series and add the assert in >> the second (2/2) patch? What should be the version number of the series? > > No, why? Simply fold this change into the earlier one, and call the > result v3. Okkk, I just realized what I have done.
diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c index 66026ad3e0..7b3377d46e 100644 --- a/xen/common/hypfs.c +++ b/xen/common/hypfs.c @@ -377,8 +377,10 @@ 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) ) + if ( e_namelen >= sizeof(name) ) { + ASSERT_UNREACHABLE(); 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. Also, for debug builds, add an assertion to ensure that the assumption made on the size of the destination buffer still holds. Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com> --- Changes in v2: - add ASSERT_UNREACHABLE() - update commit message accordingly xen/common/hypfs.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)