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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <520885538.3699431.1686240731083.JavaMail.zimbra@nod.at>
Date: Thu, 8 Jun 2023 18:12:11 +0200 (CEST)
From: Richard Weinberger <richard@....at>
To: Petr Mladek <pmladek@...e.com>
Cc: Kees Cook <keescook@...omium.org>, 
	linux-hardening <linux-hardening@...r.kernel.org>, 
	netdev <netdev@...r.kernel.org>, 
	linux-kernel <linux-kernel@...r.kernel.org>, 
	Steven Rostedt <rostedt@...dmis.org>, 
	senozhatsky <senozhatsky@...omium.org>, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
	Rasmus Villemoes <linux@...musvillemoes.dk>, 
	davem <davem@...emloft.net>, edumazet <edumazet@...gle.com>, 
	kuba <kuba@...nel.org>, pabeni <pabeni@...hat.com>, 
	Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Wedson Almeida Filho <wedsonaf@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, 
	Alexei Starovoitov <ast@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, 
	Jesper Dangaard Brouer <hawk@...nel.org>, 
	John Fastabend <john.fastabend@...il.com>
Subject: Re: [RFC PATCH 0/1] Integer overflows while scanning for integers

----- Ursprüngliche Mail -----
> Von: "Petr Mladek" <pmladek@...e.com>
> 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