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

Message-ID: <20251002174727.12242Ae2-hca@linux.ibm.com>
Date: Thu, 2 Oct 2025 19:47:27 +0200
From: Heiko Carstens <hca@...ux.ibm.com>
To: Kees Cook <kees@...nel.org>
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 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!