linux-kernel - Re: [PATCH] s390/sysinfo: Replace sprintf with snprintf for buffer safety

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: <202510020942.9BBB100C6@keescook>
Date: Thu, 2 Oct 2025 10:20:35 -0700
From: Kees Cook <kees@...nel.org>
To: Heiko Carstens <hca@...ux.ibm.com>
Cc: Josephine Pfeiffer <hi@...ie.lol>, Vasily Gorbik <gor@...ux.ibm.com>,
	Alexander Gordeev <agordeev@...ux.ibm.com>,
	Christian Borntraeger <borntraeger@...ux.ibm.com>,
	Sven Schnelle <svens@...ux.ibm.com>, linux-s390@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] s390/sysinfo: Replace sprintf with snprintf for buffer
 safety

On Thu, Oct 02, 2025 at 09:48:21AM +0200, Heiko Carstens wrote:
> On Wed, Oct 01, 2025 at 07:41:04PM +0200, Josephine Pfeiffer wrote:
> > Replace sprintf() with snprintf() when formatting symlink target name
> > to prevent potential buffer overflow. The link_to buffer is only 10
> > bytes, and using snprintf() ensures proper bounds checking if the
> > topology nesting limit value is unexpectedly large.
> > 
> > Signed-off-by: Josephine Pfeiffer <hi@...ie.lol>
> > ---
> >  arch/s390/kernel/sysinfo.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/s390/kernel/sysinfo.c b/arch/s390/kernel/sysinfo.c
> > index 1ea84e942bd4..33ca3e47a0e6 100644
> > --- a/arch/s390/kernel/sysinfo.c
> > +++ b/arch/s390/kernel/sysinfo.c
> > @@ -526,7 +526,7 @@ static __init int stsi_init_debugfs(void)
> >  	if (IS_ENABLED(CONFIG_SCHED_TOPOLOGY) && cpu_has_topology()) {
> >  		char link_to[10];
> >  
> > -		sprintf(link_to, "15_1_%d", topology_mnest_limit());
> > +		snprintf(link_to, sizeof(link_to), "15_1_%d", topology_mnest_limit());
> 
> [Adding Kees]
> 
> I don't think that patches like this will make the world a better

topology_mnest_limit() returns u8, so length will never be >3, so the
link_to is already sized to the max possible needed size. In this case
the existing code is provably _currently_ safe. If the return type of
topology_mnest_limit() ever changed, though, it would be a problem. For
that reason, I would argue that in the interests of defensiveness, the
change is good. For more discoverable robustness, you could write it as:

WARN_ON(snprintf(link_to, sizeof(link_to), "15_1_%d",
		 topology_mnest_limit()) >= sizeof(link_to))

But that starts to get pretty ugly.

> place. But you could try some macro magic and try to figure out if the
> first parameter of sprintf() is an array, and if so change the call from
> sprintf() to snprintf() transparently for all users. Some similar magic
> that has been added to strscpy() with the optional third parameter.
> 
> No idea if that is possible at all, or if that would introduce some
> breakage.

Yeah, it should be possible. I actually thought CONFIG_FORTIFY_SOURCE
already covered sprintf, but it doesn't yet. Totally untested, and
requires renaming the existing sprintf to __sprintf:

#define sprintf(dst, fmt...)					\
	__builtin_choose_expr(__is_array(dst),			\
			      snprintf(dst, sizeof(dst), fmt),	\
			      __sprintf(dst, fmt))

The return values between sprintf and snprintf should be the same,
though there is a potential behavioral difference in that dst contents
will be terminated now, so any "silent" overflows that happened to work
before (e.g. writing the \0 just past the end of a buffer into padding)
will suddenly change. Making this kind of global change could lead to a
number of hard-to-find bugs.

Doing an explicit run-time check would probably be better, which would
warn about the overflow but still allow it to happen. Again, untested:

#define sprintf(dst, fmt...)	 ({					\
	const size_t __dst_len = __builtin_dynamic_object_size(dst, 1);	\
	/* __written doesn't include \0 */				\
	const size_t __written = __sprintf(dst, frm);			\
	/* If the destination buffer size knowable, check it */		\
	if (__dst_len != SIZE_MAX &&					\
	    (!__dst_len || __dst_len - 1 < __written)			\
		WARN_ONCE("sprintf buffer overflow: %d written to buffer size %zu!\n",\
			  __written + 1, __dst_len);			\
	__written;							\
})

tl;dr: I think it's worth switching to snprintf (or scnprintf) where
possible to make an explicit choice about what the destination buffer
is expected to contain in the case of an overflow. Using sprintf leaves
it potentially ambiguous.

-Kees

-- 
Kees Cook