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!
