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

[Heiko Carstens <hca@linux.ibm.com>]

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!