Re: [RFC PATCH 0/1] Integer overflows while scanning for integers

[Linus Torvalds]

On Fri, Jun 9, 2023 at 10:03 AM Petr Mladek <pmladek@xxxxxxxx> wrote:
>
> Added Linus into CC. Quick summary for him:
>
> 1. The original problem is best described in the cover letter, see
>    https://lore.kernel.org/r/20230607223755.1610-1-richard@xxxxxx

Well, honestly, I'm not convinced this is a huge problem, but:

> > I can't figure out what POSIX actually says should or could happen with
> > sscanf("99999999999999", "%i", &x);
>
> It looks to me that it does not say anything about it. Even the latest
> IEEE Std 1003.1-2017 says just this:

We really don't need to language-lawyer POSIX. It's one of those
"let's follow it to minimize confusion" things, but no need to think
our internal library decisions need to actually care deeply.

For example, we did all the '%pXYZ" extensions on the printf() side,
which are very much *not* POSIX. They have been a huge success.

And even when it comes to the user space ABI, we've always prioritized
backwards compatibility over POSIX.

In some cases have actively disagreed with POSIX for being actively
wrong. For example, POSIX at one point thought that 'accept()',
'bind()' and friends should take a 'size_t' for the address length,
which was complete garbage.

It's "int", and I absolutely refused to follow broken specs. They
ended up fixing their completely broken spec to use 'socklen_t'
instead, which is still completely wrong (it's "int", and anything
else is wrong), but at least you can now be POSIX-compatible without
being broken.

In this case, I would suggest:

 - absolutely do *NOT* add a WARNING() for this, because that just
allows user space to arbitrarily cause kernel warnings. Not ok.

 - fail overflows by default

 - to be able to deal with any compatibility issues, add a way to say
"don't fail overflows" in the format specs, maybe using '!'.

IOW, make

     sscanf("999999999999", "%d", &i);

return 0 (because it couldn't parse a single field - go ahead and set
errno to ERANGE too if you care), but allow people to do

     sscanf("999999999999", "%!d", &i);

which would return 1 and set 'i' to -727379969.

That makes us error out on overflow by default, but gives us an easy
way to say "in this case, I'll take the overflow value" if it turns
out that we have some situation where people gave bad input and it
"just worked".

There's a special case of "overflow" that we may need to deal with:
passing in "-1" as a way to set all bits to one in an unsigned value
is not necessarily uncommon.

So I suspect that even if the conversion is something like "%lu" (or
"%x"), which is technically meant for unsigned values, we need to
still allow negative values, and just say "it's not overflow, it's 2's
complement".

                 Linus