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

[Rasmus Villemoes]

On 08/06/2023 17.27, Petr Mladek wrote:
> On Wed 2023-06-07 16:36:12, Kees Cook wrote:

> It seems that userspace implementation of sscanf() and vsscanf()
> returns -ERANGE in this case. It might be a reasonable solution.

Well. _Some_ userspace implementation does that. It's not in POSIX.
While "man scanf" lists that ERANGE error, it also explicitly says that:

CONFORMING TO
       The functions fscanf(), scanf(), and sscanf() conform to C89 and
C99 and POSIX.1-2001.  These standards do  not  specify  the
       ERANGE error.

I can't figure out what POSIX actually says should or could happen with
sscanf("99999999999999", "%i", &x);


> Well, there is a risk of introducing security problems. The error
> value might cause an underflow/overflow when the caller does not expect
> a negative value.

There is absolutely no way we can start letting sscanf() return a
negative err value, in exactly the same way we cannot possibly let
vsnprintf() do that. We can stop early, possibly with a WARNing if it's
the format string we're unhappy about ('cause that should be
compile-time constant or, e.g. in the netdevice name case, carefully
checked by the caller) and return "number of succesful conversions so
far" (scanf) / "number of bytes written to buffer" (printf).

> Alternative solution would be to update the "ip" code so that it
> reads the number separately and treat zero return value as
> -EINVAL.

The netdev naming code _could_ be updated to just not use scanf at all
or the bitmap of in-use numbers, just do the "sprintf(buf, fmt, i)" in a
loop and stop when the name is not in use. That's a win as long as there
are less than ~256 names already matching the pattern, but the
performance absolutely tanks if there are many more than that. So I
won't actually suggest that.

Rasmus