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

Xenia Ragiadakou posted 1 patch 1 year, 8 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20220804133058.1832142-1-burzalodowa@gmail.com
xen/common/hypfs.c | 5 +++++
1 file changed, 5 insertions(+)
[PATCH v3] xen/hypfs: check the return value of snprintf to avoid leaking stack accidently
Posted by Xenia Ragiadakou 1 year, 8 months ago
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

Changes in v3:
- place braces according to the coding style
- add the changes made in v1 because v2 was incremental to v1

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

diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c
index acd258edf2..cdf4ee0171 100644
--- a/xen/common/hypfs.c
+++ b/xen/common/hypfs.c
@@ -377,6 +377,11 @@ 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) )
+    {
+        ASSERT_UNREACHABLE();
+        return -ENOBUFS;
+    }
     e_len = DIRENTRY_SIZE(e_namelen);
     direntry.e.pad = 0;
     direntry.e.type = template->e.type;
-- 
2.34.1
Re: [PATCH v3] xen/hypfs: check the return value of snprintf to avoid leaking stack accidently
Posted by Juergen Gross 1 year, 8 months ago
On 04.08.22 15:30, 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