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

[Petr Mladek <pmladek@suse.com>]

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@nod.at

2. I think that we actually have two problems:

     a) How to find a code where sscanf() might get invalid output.
	Such a code might require some special/better handling
	of this situation.

     b) How to eventually provide a useful message back to
	userspace.


On Fri 2023-06-09 12:10:29, Rasmus Villemoes wrote:
> 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);

It looks to me that it does not say anything about it. Even the latest
IEEE Std 1003.1-2017 says just this:

--- cut ---
RETURN VALUE
Upon successful completion, these functions shall return the number of
successfully matched and assigned input items; this number can be zero
in the event of an early matching failure. If the input ends before
the first conversion (if any) has completed, and without a matching
failure having occurred, EOF shall be returned. If an error occurs
before the first conversion (if any) has completed, and without
a matching failure having occurred, EOF shall be returned
and errno shall be set to indicate the error. If a read error
occurs, the error indicator for the stream shall be set.

ERRORS
For the conditions under which the fscanf() functions fail and may
fail, refer to fgetc or fgetwc.

In addition, the fscanf() function shall fail if:

[EILSEQ]
Input byte sequence does not form a valid character.
[ENOMEM]
Insufficient storage space is available.
In addition, the fscanf() function may fail if:

[EINVAL]
There are insufficient arguments.
--- cut ---

I do not see anything about overflow anywhere.

Well, the -ERANGE returned by the Linux implementation looks
quite practical.

> > 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.

I agree.

> We can stop early, possibly with a WARNing if it's

Thinking more about it, the WARN() is a really big hammer even
if we introduced a never-panicking alternative, e.g. INFO()
or REPORT().

sscanf() does not know in which situation it is used and how
serious the overflow would be. I mean that it does not know
how to provide useful report and if it is needed at all.

Also WARN()-like report would only help with the 1st problem,
how find the code where the overflow might happen.
But it would not help to handle it a reasonable way.

> > 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.

Yes, sscanf() might be replaced by a tricky code to better handle possible
wrong input.

Alternatively, we could provide sscanf_with_err() [*] which would
return the error codes. It might be used in situations where
the caller is ready to handle it.

For example, __dev_alloc_name() might return -ERANGE or -EINVAL.
As a result "ip" might exit with 2 status. It means an error
from kernel. __dev_alloc_name() might eventually print some
useful message into the kernel log. Normal user would not be
able to read it. But they might ask admin to check the kernel log...


My opinion:

1. I would prefer to avoid WARN() even when a non-panic alternative
   is used. It might be too noisy.

   Well, it might be acceptable when it is enabled only with some
   CONFIG_DEBUG_SSCANF. But the question is who would enable it.
   If a developer knew that the problem might be in sscanf() then
   they probably are close to the buggy code anyway.


2. The sscanf_with_err() [*] variant looks practical to actually
   handle the wrong input.


[*] sscanf_with_error() is super ugly name. Any better idea is
    welcome.

Best Regards,
Petr