linux-kernel - Re: [PATCH] fs/orangefs: use snprintf() instead of sprintf()

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: <CAOg9mSScdVGLu6RFFe_Lcv5ugRmN8Ov-qZ=yNUM84Jv94jKz1g@mail.gmail.com>
Date: Mon, 30 Jun 2025 13:18:47 -0400
From: Mike Marshall <hubcap@...ibond.com>
To: Amir Mohammad Jahangirzad <a.jahangirzad@...il.com>
Cc: Al Viro <viro@...iv.linux.org.uk>, devel@...ts.orangefs.org, 
	linux-kernel@...r.kernel.org, Mike Marshall <hubcap@...ibond.com>
Subject: Re: [PATCH] fs/orangefs: use snprintf() instead of sprintf()

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@...ibond.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@...il.com> wrote:
> >
> > On Sun, Jun 22, 2025 at 10:18 PM Al Viro <viro@...iv.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?