[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240219152312.GD10170@google.com>
Date: Mon, 19 Feb 2024 15:23:12 +0000
From: Lee Jones <lee@...nel.org>
To: James Bottomley <jejb@...ux.ibm.com>
Cc: Geert Uytterhoeven <geert@...ux-m68k.org>, linux-kernel@...r.kernel.org,
linux-hardening@...r.kernel.org, Finn Thain <fthain@...ux-m68k.org>,
Michael Schmitz <schmitzmic@...il.com>,
"Martin K. Petersen" <martin.petersen@...cle.com>,
drew@...orado.edu, Tnx to <Thomas_Roesch@...maus.de>,
linux-scsi@...r.kernel.org
Subject: Re: [PATCH 03/10] scsi: NCR5380: Replace snprintf() with the safer
scnprintf() variant
On Sat, 10 Feb 2024, James Bottomley wrote:
> On Thu, 2024-02-08 at 10:29 +0000, Lee Jones wrote:
> > On Thu, 08 Feb 2024, Geert Uytterhoeven wrote:
> >
> > > Hi Lee,
> > >
> > > Thanks for your patch!
> > >
> > > On Thu, Feb 8, 2024 at 9:48 AM Lee Jones <lee@...nel.org> wrote:
> > > > There is a general misunderstanding amongst engineers that
> > > > {v}snprintf()
> > > > returns the length of the data *actually* encoded into the
> > > > destination
> > > > array. However, as per the C99 standard {v}snprintf() really
> > > > returns
> > > > the length of the data that *would have been* written if there
> > > > were
> > > > enough space for it. This misunderstanding has led to buffer-
> > > > overruns
> > > > in the past. It's generally considered safer to use the
> > > > {v}scnprintf()
> > > > variants in their place (or even sprintf() in simple cases). So
> > > > let's
> > > > do that.
> > >
> > > Confused... The return value is not used at all?
> >
> > Future proofing. The idea of the effort is to rid the use entirely.
> >
> > - Usage is inside a sysfs handler passing PAGE_SIZE as the size
> > - s/snprintf/sysfs_emit/
> > - Usage is inside a sysfs handler passing a bespoke value as the
> > size
> > - s/snprintf/scnprintf/
> > - Return value used, but does *not* care about overflow
> > - s/snprintf/scnprintf/
> > - Return value used, caller *does* care about overflow
> > - s/snprintf/seq_buf/
> > - Return value not used
> > - s/snprintf/scnprintf/
> >
> > This is the final case.
>
> To re-ask Geert's question: the last case can't ever lead to a bug or
> problem, what value does churning the kernel to change it provide? As
> Finn said, if we want to deprecate it as a future pattern, put it in
> checkpatch.
Adding this to checkpatch is a good idea.
What if we also take Kees's suggestion and hit all of these found in
SCSI in one patch to keep the churn down to a minimum?
--
Lee Jones [李琼斯]
Powered by blists - more mailing lists