sprintf() is discouraged for use with bounded destination buffers
as it does not prevent buffer overflows when the formatted output
exceeds the destination buffer size. snprintf() is a safer
alternative as it limits the number of bytes written and ensures
NUL-termination.

Replace sprintf() with snprintf() for copying the debug string
into a temporary buffer, using ORANGEFS_MAX_DEBUG_STRING_LEN as
the maximum size to ensure safe formatting and prevent memory
corruption in edge cases.


Signed-off-by: Amir Mohammad Jahangirzad <a.jahangirzad@gmail.com>
---
 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 f7095c91660c..e1613e0847e8 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 = sprintf(buf, "%s", (char *)file->private_data);
+	sprintf_ret = snprintf(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.43.0

On Sun, Jun 8, 2025 at 8:06 PM Amir Mohammad Jahangirzad
<a.jahangirzad@gmail.com> wrote:
>
> sprintf() is discouraged for use with bounded destination buffers
> as it does not prevent buffer overflows when the formatted output
> exceeds the destination buffer size. snprintf() is a safer
> alternative as it limits the number of bytes written and ensures
> NUL-termination.
>
> Replace sprintf() with snprintf() for copying the debug string
> into a temporary buffer, using ORANGEFS_MAX_DEBUG_STRING_LEN as
> the maximum size to ensure safe formatting and prevent memory
> corruption in edge cases.
>
>
> Signed-off-by: Amir Mohammad Jahangirzad <a.jahangirzad@gmail.com>
> ---
>  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 f7095c91660c..e1613e0847e8 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 = sprintf(buf, "%s", (char *)file->private_data);
> +       sprintf_ret = snprintf(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.43.0
>

Hi there,

Just following up to see if there's anything you'd like me to change or
address in the patch before it can move forward.

Please let me know if any updates are needed.

Regards,
Amir Mohammad Jahangirzad

On Sun, Jun 22, 2025 at 10:09:58PM +0330, Amir Mohammad Jahangirzad wrote:

> > Replace sprintf() with snprintf() for copying the debug string
> > into a temporary buffer, using ORANGEFS_MAX_DEBUG_STRING_LEN as
> > the maximum size to ensure safe formatting and prevent memory
> > corruption in edge cases.

Out of curiosity - have you actually looked at the format used there?

On Sun, Jun 22, 2025 at 10:18 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Sun, Jun 22, 2025 at 10:09:58PM +0330, Amir Mohammad Jahangirzad wrote:
>
> > > Replace sprintf() with snprintf() for copying the debug string
> > > into a temporary buffer, using ORANGEFS_MAX_DEBUG_STRING_LEN as
> > > the maximum size to ensure safe formatting and prevent memory
> > > corruption in edge cases.
>
> Out of curiosity - have you actually looked at the format used there?

No, I just found this through static analysis. Is there any issue with it?

Hi Y'all...

I was about to add Amir's patch on top of 6.16-rc3 and run it through
xfstests, when I saw Al's comment.

Al patched a similar bit of code in orangefs-debugfs.c without
removing sprintf:

45063097 - "don't open-code file_inode()"

When I look at orangefs_debug_read as it is now, I might be trusting
file->private_data's length too much and Amir's patch might risk sending
a bad sprintf_ret to simple_read_from_buffer. Al, could you be
more explicit?

-Mike

On Sun, Jun 22, 2025 at 4:10 PM Amir Mohammad Jahangirzad
<a.jahangirzad@gmail.com> wrote:
>
> On Sun, Jun 22, 2025 at 10:18 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Sun, Jun 22, 2025 at 10:09:58PM +0330, Amir Mohammad Jahangirzad wrote:
> >
> > > > Replace sprintf() with snprintf() for copying the debug string
> > > > into a temporary buffer, using ORANGEFS_MAX_DEBUG_STRING_LEN as
> > > > the maximum size to ensure safe formatting and prevent memory
> > > > corruption in edge cases.
> >
> > Out of curiosity - have you actually looked at the format used there?
>
> No, I just found this through static analysis. Is there any issue with it?

Hi Amir...

I added your patch to 6.16-rc3 and ran it through xfstests, it
works fine. snprintf won't overflow the buffer and gives you
the opportunity to compare the return code to the buffer size
to detect truncation. It doesn't look like many of the other uses
of snprintf in the kernel bother with checking for truncation.

Whatever Al was pointing out is probably important... :-)

-Mike

On Mon, Jun 23, 2025 at 1:02 PM Mike Marshall <hubcap@omnibond.com> wrote:
>
> Hi Y'all...
>
> I was about to add Amir's patch on top of 6.16-rc3 and run it through
> xfstests, when I saw Al's comment.
>
> Al patched a similar bit of code in orangefs-debugfs.c without
> removing sprintf:
>
> 45063097 - "don't open-code file_inode()"
>
> When I look at orangefs_debug_read as it is now, I might be trusting
> file->private_data's length too much and Amir's patch might risk sending
> a bad sprintf_ret to simple_read_from_buffer. Al, could you be
> more explicit?
>
> -Mike
>
> On Sun, Jun 22, 2025 at 4:10 PM Amir Mohammad Jahangirzad
> <a.jahangirzad@gmail.com> wrote:
> >
> > On Sun, Jun 22, 2025 at 10:18 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> > >
> > > On Sun, Jun 22, 2025 at 10:09:58PM +0330, Amir Mohammad Jahangirzad wrote:
> > >
> > > > > Replace sprintf() with snprintf() for copying the debug string
> > > > > into a temporary buffer, using ORANGEFS_MAX_DEBUG_STRING_LEN as
> > > > > the maximum size to ensure safe formatting and prevent memory
> > > > > corruption in edge cases.
> > >
> > > Out of curiosity - have you actually looked at the format used there?
> >
> > No, I just found this through static analysis. Is there any issue with it?