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

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

[Josephine Pfeiffer]

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@josie.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());
 		debugfs_create_symlink("topology", stsi_root, link_to);
 	}
 	return 0;
-- 
2.51.0

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

[Heiko Carstens]

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

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

[Kees Cook]

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

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

[Heiko Carstens]

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!