Received-SPF: pass (google.com: domain of linux-kernel+bounces-60406-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <98bdd564c6bf1894717d060f3187c779e969fc5f.camel@linux.ibm.com>
Subject: Re: [PATCH 03/10] scsi: NCR5380: Replace snprintf() with the safer
 scnprintf() variant
From: James Bottomley <jejb@linux.ibm.com>
Reply-To: jejb@linux.ibm.com
To: Lee Jones <lee@kernel.org>, Geert Uytterhoeven <geert@linux-m68k.org>
Cc: 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
Date: Sat, 10 Feb 2024 07:56:38 -0500
In-Reply-To: <20240208102939.GF689448@google.com>
References: <20240208084512.3803250-1-lee@kernel.org>
	 <20240208084512.3803250-4-lee@kernel.org>
	 <CAMuHMdX72mpGgb3Wp0WRX3V78nn+bWUqiYz25CjeMNPpWaPmxg@mail.gmail.com>
	 <20240208102939.GF689448@google.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

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

James