linux-kernel - Re: [PATCH v6] checkpatch: add check for snprintf to scnprintf

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240502092943.GG5338@google.com>
Date: Thu, 2 May 2024 10:29:43 +0100
From: Lee Jones <lee@...nel.org>
To: Joe Perches <joe@...ches.com>
Cc: Kees Cook <keescook@...omium.org>,
	Justin Stitt <justinstitt@...gle.com>,
	Andy Whitcroft <apw@...onical.com>,
	Dwaipayan Ray <dwaipayanray1@...il.com>,
	Lukas Bulwahn <lukas.bulwahn@...il.com>,
	linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org,
	Finn Thain <fthain@...ux-m68k.org>
Subject: Re: [PATCH v6] checkpatch: add check for snprintf to scnprintf

On Mon, 29 Apr 2024, Joe Perches wrote:

> On Mon, 2024-04-29 at 12:49 -0700, Kees Cook wrote:
> > On Mon, Apr 29, 2024 at 06:39:28PM +0000, Justin Stitt wrote:
> > > I am going to quote Lee Jones who has been doing some snprintf ->
> > > scnprintf refactorings:
> > > 
> > > "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."
> > > 
> > > To help prevent new instances of snprintf() from popping up, let's add a
> > > check to checkpatch.pl.
> > > 
> > > Suggested-by: Finn Thain <fthain@...ux-m68k.org>
> > > Signed-off-by: Justin Stitt <justinstitt@...gle.com>
> > 
> > Thanks!
> > 
> > Reviewed-by: Kees Cook <keescook@...omium.org>
> > 
> 
> $ git grep -P '\b((v|)snprintf)\s*\(' | wc -l
> 7745
> $ git grep -P '(?:return\s+|=\s*)\b((v|)snprintf)\s*\(' | wc -l
> 1626
> 
> Given there are ~5000 uses of these that don't care
> whether or not it's snprintf or scnprintf, I think this
> is not great.
> 
> I'd much rather make sure the return value of the call
> is used before suggesting an alternative.
> 
> $ git grep  -P '\b((v|)snprintf)\s*\(.*PAGE_SIZE' | wc -l
> 515
> 
> And about 1/3 of these snprintf calls are for sysfs style
> output that ideally would be converted to sysfs_emit or
> sysfs_emit_at instead.

I am working on the migration of these (this patch was spun off from
that project in fact).  Some subsystems are currently prioritising the
status quo (a.k.a. "no churn"), but most have been accepting of the
changes.

Planning to get back to it once the CVE project has calmed a little.

Those numbers should diminish over time.

-- 
Lee Jones [李琼斯]