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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAOg9mSSnnLf_uoM1pQp4yMjd4e=q07PjBWw62ch3P+V1c--+Dw@mail.gmail.com>
Date: Sat, 19 Jul 2025 09:29:43 -0400
From: Mike Marshall <hubcap@...ibond.com>
To: Dan Carpenter <dan.carpenter@...aro.org>, Mike Marshall <hubcap@...ibond.com>
Cc: Amir Mohammad Jahangirzad <a.jahangirzad@...il.com>, Martin Brandenburg <martin@...ibond.com>, 
	devel@...ts.orangefs.org, linux-kernel@...r.kernel.org, 
	kernel-janitors@...r.kernel.org
Subject: Re: [PATCH next] fs/orangefs: Use scnprintf() instead of snprintf()

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@...aro.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@...aro.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
>