[PATCH next] fs/orangefs: Use scnprintf() instead of snprintf()

[PATCH next] fs/orangefs: Use scnprintf() instead of snprintf()

[Dan Carpenter]

We recently changed this code from using sprintf() to using snprintf()
as a kernel hardening measure.  However, that's still not ideal.  The
snprintf() function returns the number of bytes which *would* have been
copied if we had enough space while the snprintf() function returns the
number of bytes which are *actually* copied.

So if there were an overflow, the conversion to snprintf() would prevent
memory corruption but it would still leave an information leak where we
would read beyond the end of "buf".  Use scnprintf() to fix both the
write overflow and the read overflow.

Fixes: fc08e0b8f099 ("fs/orangefs: use snprintf() instead of sprintf()")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 fs/orangefs/orangefs-debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/orangefs/orangefs-debugfs.c b/fs/orangefs/orangefs-debugfs.c
index a5fad515815e..e463d3c73533 100644
--- a/fs/orangefs/orangefs-debugfs.c
+++ b/fs/orangefs/orangefs-debugfs.c
@@ -396,7 +396,7 @@ static ssize_t orangefs_debug_read(struct file *file,
 		goto out;
 
 	mutex_lock(&orangefs_debug_lock);
-	sprintf_ret = snprintf(buf, ORANGEFS_MAX_DEBUG_STRING_LEN, "%s", (char *)file->private_data);
+	sprintf_ret = scnprintf(buf, ORANGEFS_MAX_DEBUG_STRING_LEN, "%s", (char *)file->private_data);
 	mutex_unlock(&orangefs_debug_lock);
 
 	read_ret = simple_read_from_buffer(ubuf, count, ppos, buf, sprintf_ret);
-- 
2.47.2

Re: [PATCH next] fs/orangefs: Use scnprintf() instead of snprintf()

[Mike Marshall]

Thanks. I noticed the "returns the number of bytes
which *would* have been copied" issue when I read the
snprintf man page while looking at the patch. I'll see
about making this change in my "next" before the
rc period is over...

Also, while looking at this patch, I made
an xfstest that includes buffer overflow attempts
and other bogus input. It would be, of course,
an orangefs specific xfstest... I wonder if I
should try to get it pulled into the main xfstests
repo?

-Mike

On Fri, Jul 18, 2025 at 5:24 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> We recently changed this code from using sprintf() to using snprintf()
> as a kernel hardening measure.  However, that's still not ideal.  The
> snprintf() function returns the number of bytes which *would* have been
> copied if we had enough space while the snprintf() function returns the
> number of bytes which are *actually* copied.
>
> So if there were an overflow, the conversion to snprintf() would prevent
> memory corruption but it would still leave an information leak where we
> would read beyond the end of "buf".  Use scnprintf() to fix both the
> write overflow and the read overflow.
>
> Fixes: fc08e0b8f099 ("fs/orangefs: use snprintf() instead of sprintf()")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  fs/orangefs/orangefs-debugfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/orangefs/orangefs-debugfs.c b/fs/orangefs/orangefs-debugfs.c
> index a5fad515815e..e463d3c73533 100644
> --- a/fs/orangefs/orangefs-debugfs.c
> +++ b/fs/orangefs/orangefs-debugfs.c
> @@ -396,7 +396,7 @@ static ssize_t orangefs_debug_read(struct file *file,
>                 goto out;
>
>         mutex_lock(&orangefs_debug_lock);
> -       sprintf_ret = snprintf(buf, ORANGEFS_MAX_DEBUG_STRING_LEN, "%s", (char *)file->private_data);
> +       sprintf_ret = scnprintf(buf, ORANGEFS_MAX_DEBUG_STRING_LEN, "%s", (char *)file->private_data);
>         mutex_unlock(&orangefs_debug_lock);
>
>         read_ret = simple_read_from_buffer(ubuf, count, ppos, buf, sprintf_ret);
> --
> 2.47.2
>