Received-SPF: pass (google.com: domain of linux-kernel+bounces-72556-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 20 Feb 2024 08:28:16 +0000
From: Lee Jones <lee@kernel.org>
To: James Bottomley <jejb@linux.ibm.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>, linux-kernel@vger.kernel.org,
	linux-hardening@vger.kernel.org, Finn Thain <fthain@linux-m68k.org>,
	Michael Schmitz <schmitzmic@gmail.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	drew@colorado.edu, Tnx to <Thomas_Roesch@m2.maus.de>,
	linux-scsi@vger.kernel.org
Subject: Re: [PATCH 03/10] scsi: NCR5380: Replace snprintf() with the safer
 scnprintf() variant
Message-ID: <20240220082816.GI10170@google.com>
References: <20240208084512.3803250-1-lee@kernel.org>
 <20240208084512.3803250-4-lee@kernel.org>
 <CAMuHMdX72mpGgb3Wp0WRX3V78nn+bWUqiYz25CjeMNPpWaPmxg@mail.gmail.com>
 <20240208102939.GF689448@google.com>
 <98bdd564c6bf1894717d060f3187c779e969fc5f.camel@linux.ibm.com>
 <20240219152312.GD10170@google.com>
 <f5bef33da6c50e5fd067577f16f460025fe9a601.camel@linux.ibm.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <f5bef33da6c50e5fd067577f16f460025fe9a601.camel@linux.ibm.com>

On Mon, 19 Feb 2024, James Bottomley wrote:

> On Mon, 2024-02-19 at 15:23 +0000, Lee Jones wrote:
> > 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@kernel.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
> > > orproblem, 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?
> 
> That doesn't fix the churn problem because you're still changing the
> source.  For ancient drivers, we keep the changes to a minimum to avoid
> introducing inadvertent bugs which aren't discovered until months
> later.  If there's no actual bug in the driver, there's no reason to
> change the code.

Okay, no problem.  Would you like me to drop these from the set and
resubmit or are you happy to cherry-pick the remainder?

-- 
Lee Jones [李琼斯]