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

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Petr Mladek" <pmladek@xxxxxxxx>
> On Wed 2023-06-07 16:36:12, Kees Cook wrote:
>> On Thu, Jun 08, 2023 at 12:37:54AM +0200, Richard Weinberger wrote:
>> > Hi!
>> > 
>> > Lately I wondered whether users of integer scanning functions check
>> > for overflows.
>> > To detect such overflows around scanf I came up with the following
>> > patch. It simply triggers a WARN_ON_ONCE() upon an overflow.
>> > 
>> > After digging into various scanf users I found that the network device
>> > naming code can trigger an overflow.
>> > 
>> > e.g:
>> > $ ip link add 1 type veth peer name 9999999999
>> > $ ip link set name "%d" dev 1
>> > 
>> > It will trigger the following WARN_ON_ONCE():
>> > ------------[ cut here ]------------
>> > WARNING: CPU: 2 PID: 433 at lib/vsprintf.c:3701 vsscanf+0x6ce/0x990
>> 
>> Hm, it's considered a bug if a WARN or BUG can be reached from
>> userspace,
> 
> Good point. WARN() does not look like the right way in this case.

Well, the whole point of my RFC(!) patch is showing the issue and providing a way
to actually find such call sites, like the netdev code.
 
> Another problem is that some users use panic_on_warn. In this case,
> the above "ip" command calls would trigger panic(). And it does not
> look like an optimal behavior.

Only if we don't fix netdev code.
 
> I know there already are some WARN_ONs for similar situations, e.g.
> set_field_width() or set_precision(). But these do not get random
> values. And it would actually be nice to introduce something like
> INFO() that would be usable for these less serious problems where
> the backtrace is useful but they should never trigger panic().
> 
>> so this probably needs to be rearranged (or callers fixed).
>> Do we need to change the scanf API for sane use inside the kernel?
> 
> It seems that userspace implementation of sscanf() and vsscanf()
> returns -ERANGE in this case. It might be a reasonable solution.
> 
> 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.

Agreed. Without inspecting all users of scanf we cannot change the API.

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

The kernel needs fixing, not userspace.

Thanks,
//richard