WARNING in ex_handler_uaccess

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 10b82d51 Merge branch 'for-5.9-fixes' of git://git.kernel...
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=13fb6b07900000
kernel config: https://syzkaller.appspot.com/x/.config?x=773ee8ece5f19a24
dashboard link: https://syzkaller.appspot.com/bug?extid=ea3a78a71705faf41d77
compiler: gcc (GCC) 10.1.0-syz 20200507
userspace arch: i386

Unfortunately, I don't have any reproducer for this issue yet.

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+ea3a78...@syzkaller.appspotmail.com

WARNING: CPU: 2 PID: 6875 at arch/x86/mm/extable.c:77 ex_handler_uaccess+0xba/0xe0 arch/x86/mm/extable.c:77
Kernel panic - not syncing: panic_on_warn set ...
CPU: 2 PID: 6875 Comm: io_uring-sq Not tainted 5.9.0-rc5-syzkaller #0
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.org 04/01/2014
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x198/0x1fd lib/dump_stack.c:118
panic+0x382/0x7fb kernel/panic.c:231
__warn.cold+0x20/0x4b kernel/panic.c:600
report_bug+0x1bd/0x210 lib/bug.c:198
handle_bug+0x38/0x90 arch/x86/kernel/traps.c:234
exc_invalid_op+0x14/0x40 arch/x86/kernel/traps.c:254
asm_exc_invalid_op+0x12/0x20 arch/x86/include/asm/idtentry.h:536
RIP: 0010:ex_handler_uaccess+0xba/0xe0 arch/x86/mm/extable.c:77
Code: 1d 5c 30 fa 09 31 ff 89 de e8 e2 ae 40 00 84 db 75 80 e8 b9 b2 40 00 48 c7 c7 e0 1a 69 88 c6 05 3c 30 fa 09 01 e8 a8 ee 10 00 <0f> 0b e9 61 ff ff ff 48 89 df e8 87 7a 81 00 eb 87 e8 10 7b 81 00
RSP: 0018:ffffc9000e03f6c8 EFLAGS: 00010282
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: ffff88801b420400 RSI: ffffffff815f5a85 RDI: fffff52001c07ecb
RBP: ffffc9000e03f7b8 R08: 0000000000000001 R09: ffffffff8ce2daef
R10: 000000000000143b R11: 0000000000000000 R12: ffffffff89b3f410
R13: 000000000000000d R14: 0000000000000000 R15: 0000000000000000
fixup_exception+0x9a/0xca arch/x86/mm/extable.c:166
__exc_general_protection arch/x86/kernel/traps.c:557 [inline]
exc_general_protection+0xeb/0x2e0 arch/x86/kernel/traps.c:524
asm_exc_general_protection+0x1e/0x30 arch/x86/include/asm/idtentry.h:532
RIP: 0010:check_zeroed_user+0xd7/0x260 lib/usercopy.c:65
Code: ab 00 00 00 e8 6a 61 d4 fd 48 89 ee 48 89 df e8 5f 5d d4 fd 48 39 eb 0f 87 92 00 00 00 e8 51 61 d4 fd 0f 01 cb 0f ae e8 31 ed <49> 8b 1e 31 ff 89 ee e8 9d 5d d4 fd 85 ed 75 6d e8 34 61 d4 fd 31
RSP: 0018:ffffc9000e03f860 EFLAGS: 00050246
RAX: 0000000000000000 RBX: 2000024020012545 RCX: ffffffff83a1de61
RDX: ffff88801b420400 RSI: ffffffff83a1de6f RDI: 0000000000000006
RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffff8b34a68f
R10: 2000024020012545 R11: 0000000000000000 R12: 0000000000000000
R13: 000000000001232d R14: 2000024020000218 R15: 0000000000000000
copy_struct_from_user include/linux/uaccess.h:310 [inline]
io_openat2_prep fs/io_uring.c:3560 [inline]
io_openat2_prep+0x142/0x1a0 fs/io_uring.c:3547
io_issue_sqe+0x1932/0x61e0 fs/io_uring.c:5850
__io_queue_sqe+0x280/0x1160 fs/io_uring.c:6150
io_queue_sqe+0x692/0xfa0 fs/io_uring.c:6229
io_submit_sqe fs/io_uring.c:6299 [inline]
io_submit_sqes+0x1761/0x2400 fs/io_uring.c:6496
io_sq_thread+0x3ac/0xe00 fs/io_uring.c:6633
kthread+0x3b5/0x4a0 kernel/kthread.c:292
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294
Kernel Offset: disabled
Rebooting in 86400 seconds..


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

[Andy Lutomirski]

Hi Aleksa-

check_zeroed_user() looks buggy. It does:

if (!user_access_begin(from, size))
return -EFAULT;

unsafe_get_user(val, (unsigned long __user *) from, err_fault);

This is wrong if size < sizeof(unsigned long) -- you read outside the
area you verified using user_access_begin(). The code down below in
the loop appears to have the same bug.

I'm not sure what the right fix is. Even changing the
user_access_begin() isn't going to fix it, because you have a second
bug: this over-read can also get -EFAULT and fail a perfectly valid
syscall if the zeroed region is unaligned and butts up to the end of a
vma. For example, if you do a syscall that has a checked zeroed
region that's exactly one byte long at the last byte of a page, you'll
-EFAULT. Whoops.

I don't *think* this is a major security bug, but it does give
malicious userspace a trivial way to generate WARN messages on x86_64.
(Although I admit I'm a wee bit confused as to how this actually
triggered the #GP warning -- maybe the address check is a bit looser
than it could be. We don't actually allow tasks the allocate the
topmost lower-half canonical address.)

[Al Viro]

On Fri, Sep 18, 2020 at 04:31:33PM -0700, Andy Lutomirski wrote:

> check_zeroed_user() looks buggy. It does:
>
> if (!user_access_begin(from, size))
> return -EFAULT;
>
> unsafe_get_user(val, (unsigned long __user *) from, err_fault);
>
> This is wrong if size < sizeof(unsigned long) -- you read outside the
> area you verified using user_access_begin().

Read the code immediately prior to that. from will be word-aligned,
and size will be extended accordingly. If the area acceptable for
user_access_begin() ends *NOT* on a word boundary, you have a problem
and I would strongly recommend to seek professional help.

All reads in that thing are word-aligned and word-sized. So I very
much doubt that your analysis is correct.

[Andy Lutomirski]

Maybe -ETOOTIRED, but I seriously question the math in here. Suppose
from == (unsigned long *)1 and size == 1. Then align is 1, and we do:

from -= align;
size += align;

So now from = 0 and size = 2. Now we do user_access_begin(0, 2) and
then immediately read 4 or 8 bytes. No good.


--Andy

[Al Viro]

Could you explain what kind of insane hardware manages to do #PF-related
checks (including SMAP, whatever) with *sub*WORD* granularity?

If it's OK with 16bit read from word-aligned address, but barfs on 64bit
one... I want to know what the hell had its authors been smoking.

[David Laight]

From: Al Viro
> Sent: 19 September 2020 01:17

Not going to happen to anything in the data cache. But...

Byte parity errors on memory locations that have never been written
but power-up initialised to 'bad parity'?

I have seen that - but I've completely forgotten the hardware.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[Rasmus Villemoes]

So, not sure how the above got triggered, but I notice there might be an
edge case in check_zeroed_user():

from -= align;
size += align;

if (!user_read_access_begin(from, size))

return -EFAULT;

unsafe_get_user(val, (unsigned long __user *) from, err_fault);

Suppose size is (size_t)-3 and align is 3. What's the convention for
access_ok(whatever, 0)? Is that equivalent to access_ok(whatever, 1), or
is it always true (or $ARCH-dependent)?

But, AFAICT, no current caller of check_zeroed_user can end up passing
in a size that can overflow to 0. E.g. for the case at hand, size cannot
be more than SIZE_MAX-24.

Rasmus

[David Laight]

From: Rasmus Villemoes
> Sent: 21 September 2020 11:22

Doesn't matter, it will be doing access_ok(xxx, 8) regardless of
the user-supplied transfer length.

> But, AFAICT, no current caller of check_zeroed_user can end up passing
> in a size that can overflow to 0. E.g. for the case at hand, size cannot
> be more than SIZE_MAX-24.

Basically KASAN doesn't like you reading full words and masking
off the unused bytes.

[Al Viro]

On Mon, Sep 21, 2020 at 12:22:19PM +0200, Rasmus Villemoes wrote:

> So, not sure how the above got triggered, but I notice there might be an
> edge case in check_zeroed_user():
>
> from -= align;
> size += align;
>
> if (!user_read_access_begin(from, size))
> return -EFAULT;
>
> unsafe_get_user(val, (unsigned long __user *) from, err_fault);
>
>
> Suppose size is (size_t)-3 and align is 3. What's the convention for
> access_ok(whatever, 0)? Is that equivalent to access_ok(whatever, 1), or
> is it always true (or $ARCH-dependent)?

It's usually true...

> But, AFAICT, no current caller of check_zeroed_user can end up passing
> in a size that can overflow to 0. E.g. for the case at hand, size cannot
> be more than SIZE_MAX-24.

Might be worth slapping if (unlikely(!size)) return -EFAULT; // overflow
just before user_read_access_begin() to be sure...

[syzbot]

Auto-closing this bug as obsolete.
Crashes did not happen for a while, no reproducer and no activity.