Subject: Re: [PATCH] s390/sysinfo: Replace sprintf with snprintf for buffer safety

From: Heiko Carstens <[email-masked]>
Date: Thu, 2 Oct 2025 19:47:27 +0200

On Thu, Oct 02, 2025 at 10:20:35AM -0700, Kees Cook wrote:
> 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:
> > > -		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.

If you take the context into account: the returned value of
topology_mnest_limit() cannot be larger than 6, but that's
not obvious for anybody not familiar with the code.

So taking your feedback into account I guess I will apply
this and similar patches, even though it seems to be quite
pointless. :)

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

Ok, agreed, there is all kind of odd code around, so it is probably not a
good idea to do such a global change.

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

Thank you for taking the time to look into this!

---

From: Kees Cook <[email-masked]>
Date: Thu, 2 Oct 2025 10:20:35 -0700

On Thu, Oct 02, 2025 at 09:48:21AM +0200, Heiko Carstens 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 <[email-masked]>
> > ---
> >  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];
> >  

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

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;							\
})

-Kees

-- 
Kees Cook

---

From: Heiko Carstens <[email-masked]>
Date: Thu, 2 Oct 2025 19:47:27 +0200

---

From: Heiko Carstens <[email-masked]>
Date: Thu, 2 Oct 2025 09:48:21 +0200

[Adding Kees]

---

From: Heiko Carstens <[email-masked]>
Date: Thu, 2 Oct 2025 09:48:21 +0200

[Adding Kees]

---

From: Josephine Pfeiffer <[email-masked]>
Date: Wed, 1 Oct 2025 19:41:04 +0200

-		sprintf(link_to, "15_1_%d", topology_mnest_limit());
+		snprintf(link_to, sizeof(link_to), "15_1_%d", topology_mnest_limit());
 		debugfs_create_symlink("topology", stsi_root, link_to);
 	}
 	return 0;
-- 
2.51.0