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
Modules linked in:
CPU: 2 PID: 433 Comm: ip Not tainted 6.4.0-rc5-00025-g12c9a6293a89-dirty #27
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.15.0-0-g2dd4b9b-rebuilt.opensuse.org 04/01/2014
RIP: 0010:vsscanf+0x6ce/0x990
Code: 8d 28 ff ff ff 0f 0b e9 21 ff ff ff 48 83 ee 01 e9 4c ff ff ff 0f 0b e9 ad fe ff ff 0f 0b e9 a6 fe ff ff 0f 0b e9 03 ff ff ff <0f> 0b e9 13 fb ff ff 0f 0b e9 0c fb ff ff 0f 0b e9 ee fe ff ff e8
RSP: 0018:ffffabd4405c3680 EFLAGS: 00010206
RAX: 00000002540be3ff RBX: 0000000000000000 RCX: ffffa160052cefff
RDX: 0000000000000000 RSI: 000000000000000a RDI: ffffa15f852cf00a
RBP: ffffa15f852cf000 R08: 0000000000000009 R09: 00000002540be3ff
R10: 000000000000000a R11: 000000000000000a R12: ffffffff83f66c20
R13: ffffabd4405c36f8 R14: 00000000ffffffff R15: 0000000000000001
FS: 00007f9cf488f680(0000) GS:ffffa15fbbd00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000556cc48e0e90 CR3: 00000001085ae000 CR4: 00000000000006e0
Call Trace:
<TASK>
? __warn+0x78/0x130
? vsscanf+0x6ce/0x990
? report_bug+0xf8/0x1e0
? handle_bug+0x3f/0x70
? exc_invalid_op+0x13/0x60
? asm_exc_invalid_op+0x16/0x20
? vsscanf+0x6ce/0x990
sscanf+0x49/0x70
dev_alloc_name_ns+0x140/0x230
dev_change_name+0x8e/0x2e0
? skb_queue_tail+0x16/0x50
do_setlink+0x29e/0x11a0
? rtnl_getlink+0x396/0x3d0
? __nla_validate_parse.part.7+0x52/0xd50
__rtnl_newlink+0x496/0x8e0
? avc_has_perm_noaudit+0xaa/0x130
? __kmem_cache_alloc_node+0x36/0x180
rtnl_newlink+0x3e/0x60
rtnetlink_rcv_msg+0x13b/0x3b0
? rep_movs_alternative+0x33/0xd0
? __pfx_rtnetlink_rcv_msg+0x10/0x10
netlink_rcv_skb+0x51/0x100
netlink_unicast+0x1b6/0x280
netlink_sendmsg+0x360/0x4c0
sock_sendmsg+0x8d/0x90
____sys_sendmsg+0x1ea/0x260
___sys_sendmsg+0x83/0xd0
? folio_add_lru+0x29/0x50
? do_wp_page+0x7de/0xca0
? nfulnl_rcv_nl_event+0x2c/0xa0
? __inode_wait_for_writeback+0x7a/0xf0
? fsnotify_grab_connector+0x46/0x90
? fsnotify_destroy_marks+0x1f/0x150
? __call_rcu_common.constprop.85+0x92/0x360
? __sys_sendmsg+0x59/0xa0
__sys_sendmsg+0x59/0xa0
? exit_to_user_mode_prepare+0x27/0x120
do_syscall_64+0x3a/0x90
entry_SYSCALL_64_after_hwframe+0x72/0xdc
RIP: 0033:0x7f9cf3d72033
Code: 0f 1f 80 00 00 00 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 64 8b 04 25 18 00 00 00 85 c0 75 14 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 55 f3 c3 0f 1f 00 41 54 55 41 89 d4 53 48 89
RSP: 002b:00007ffe7d50a8e8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 000000006480fe41 RCX: 00007f9cf3d72033
RDX: 0000000000000000 RSI: 00007ffe7d50a950 RDI: 0000000000000003
RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000007
R10: 00007f9cf3c60468 R11: 0000000000000246 R12: 0000000000000001
R13: 0000556cc4897540 R14: 00007ffe7d50aa20 R15: 0000000000000005
</TASK>
---[ end trace 0000000000000000 ]---
Underflowing is also possible:
$ ip link add 1 type veth peer name -9999999999
$ ip link set name "%d" dev 1
Luckily in __dev_alloc_name() the overflow is currently harmless because
the function uses sscanf() only to construct the inuse bitmap.
While the overflow will cause wrong bits set the selected interface
name is later explicitly checked for being available.
But who knows what happens when the code is changed in future, at least
to me it was not clear that there overflows can happen.
So I think as part of kernel hardening we should offer a way to detect
overflows in scanf.
Richard Weinberger (1):
vsprintf: Warn on integer scanning overflows
lib/vsprintf.c | 33 +++++++++++++++++++++++++--------
1 file changed, 25 insertions(+), 8 deletions(-)
--
2.35.3
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, so this probably needs to be rearranged (or callers fixed).
Do we need to change the scanf API for sane use inside the kernel?
-Kees
--
Kees Cook
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.
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.
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.
Alternative solution would be to update the "ip" code so that it
reads the number separately and treat zero return value as
-EINVAL.
Best Regards,
Petr
On Thu, Jun 08, 2023 at 05:27:40PM +0200, Petr Mladek wrote:
> 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.
>
> 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.
"some users" == "most major cloud providers and a few billion Android
phones" So in pure numbers, the huge majority of Linux systems running
in the world have that option enabled.
So please don't use WARN() to catch issues that can be triggered by
userspace, that can cause data loss and worse at times.
thanks,
greg k-h
----- Ursprüngliche Mail -----
> Von: "Greg Kroah-Hartman" <[email protected]>
> "some users" == "most major cloud providers and a few billion Android
> phones" So in pure numbers, the huge majority of Linux systems running
> in the world have that option enabled.
>
> So please don't use WARN() to catch issues that can be triggered by
> userspace, that can cause data loss and worse at times.
Sorry for being unclear. My goal is not having the WARN patch immediately
applied without fixing known call sites which can trigger it.
Thanks,
//richard
----- Ursprüngliche Mail -----
> Von: "Petr Mladek" <[email protected]>
> 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
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
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/[email protected]
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
On Fri, Jun 9, 2023 at 10:03 AM Petr Mladek <[email protected]> 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/[email protected]
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
From: Rasmus Villemoes
> Sent: 09 June 2023 11:10
>
> 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);
Possibly 'undefined behaviour' - they like that one.
But I'm sure I remember the ToG 'Unix' definition not requiring that
'utilities' check for overflow on numeric command line parameters.
(It might even have said they wouldn't check.)
So it was perfectly valid for a stupidly out of range value
to be treated as a different (possibly valid) value.
What is clearly wrong is to just stop processing the
input string.
sscanf("9999999999999999999scale", "%i%s", &x, &scale)
writing any '9' to scale is clearly broken.
Personally I avoid scanf() - it is far too easy for it to do
something you didn't really intend.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)