[syzbot] general protection fault in _parse_integer_fixup_radix

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: a72b55bc981b Add linux-next specific files for 20221019
git tree: linux-next
console+strace: https://syzkaller.appspot.com/x/log.txt?x=1728c644880000
kernel config: https://syzkaller.appspot.com/x/.config?x=200524babbc01b2a
dashboard link: https://syzkaller.appspot.com/bug?extid=db1d2ea936378be0e4ea
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=12afb08c880000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11001c72880000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/fa24fb5893fd/disk-a72b55bc.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/cf1b7e7b579c/vmlinux-a72b55bc.xz

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

general protection fault, probably for non-canonical address 0xdffffc0000000000: 0000 [#1] PREEMPT SMP KASAN
KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
CPU: 1 PID: 3606 Comm: syz-executor141 Not tainted 6.1.0-rc1-next-20221019-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 09/22/2022
RIP: 0010:_parse_integer_fixup_radix+0x60/0x290 lib/kstrtox.c:29
Code: 02 00 00 41 8b 2c 24 31 ff 89 ee e8 4a ae 76 fd 85 ed 75 62 e8 71 b1 76 fd 48 89 da 48 b8 00 00 00 00 00 fc ff df 48 c1 ea 03 <0f> b6 04 02 48 89 da 83 e2 07 38 d0 7f 08 84 c0 0f 85 e2 01 00 00
RSP: 0018:ffffc90003d3fa40 EFLAGS: 00010246
RAX: dffffc0000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff8405d49f RDI: 0000000000000005
RBP: 0000000000000000 R08: 0000000000000005 R09: 0000000000000000
R10: 0000000000000000 R11: 000000000008c001 R12: ffffc90003d3fa90
R13: ffffc90003d3fbd8 R14: 000000007fffffff R15: ffff8880798524d0
FS: 000055555732c300(0000) GS:ffff8880b9b00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020000000 CR3: 000000007b427000 CR4: 00000000003506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
simple_strntoull+0x88/0x160 lib/vsprintf.c:68
memparse+0x72/0x180 lib/cmdline.c:154
shmem_parse_one+0x6a3/0xa60 mm/shmem.c:3478
vfs_parse_fs_param fs/fs_context.c:148 [inline]
vfs_parse_fs_param+0x1f9/0x3c0 fs/fs_context.c:129
vfs_parse_fs_string+0xdb/0x170 fs/fs_context.c:191
shmem_parse_options+0x15f/0x240 mm/shmem.c:3570
do_new_mount fs/namespace.c:3036 [inline]
path_mount+0x12de/0x1e20 fs/namespace.c:3370
do_mount fs/namespace.c:3383 [inline]
__do_sys_mount fs/namespace.c:3591 [inline]
__se_sys_mount fs/namespace.c:3568 [inline]
__x64_sys_mount+0x27f/0x300 fs/namespace.c:3568
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7f012fe73e09
Code: 28 c3 e8 2a 14 00 00 66 2e 0f 1f 84 00 00 00 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffed54dd188 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 0030656c69662f2e RCX: 00007f012fe73e09
RDX: 0000000020000040 RSI: 0000000020000000 RDI: 0000000000000000
RBP: 00007f012fe37d20 R08: 0000000020000180 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f012fe37db0
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
</TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
RIP: 0010:_parse_integer_fixup_radix+0x60/0x290 lib/kstrtox.c:29
Code: 02 00 00 41 8b 2c 24 31 ff 89 ee e8 4a ae 76 fd 85 ed 75 62 e8 71 b1 76 fd 48 89 da 48 b8 00 00 00 00 00 fc ff df 48 c1 ea 03 <0f> b6 04 02 48 89 da 83 e2 07 38 d0 7f 08 84 c0 0f 85 e2 01 00 00
RSP: 0018:ffffc90003d3fa40 EFLAGS: 00010246
RAX: dffffc0000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff8405d49f RDI: 0000000000000005
RBP: 0000000000000000 R08: 0000000000000005 R09: 0000000000000000
R10: 0000000000000000 R11: 000000000008c001 R12: ffffc90003d3fa90
R13: ffffc90003d3fbd8 R14: 000000007fffffff R15: ffff8880798524d0
FS: 000055555732c300(0000) GS:ffff8880b9b00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020000000 CR3: 000000007b427000 CR4: 00000000003506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
----------------
Code disassembly (best guess):
0: 02 00 add (%rax),%al
2: 00 41 8b add %al,-0x75(%rcx)
5: 2c 24 sub $0x24,%al
7: 31 ff xor %edi,%edi
9: 89 ee mov %ebp,%esi
b: e8 4a ae 76 fd callq 0xfd76ae5a
10: 85 ed test %ebp,%ebp
12: 75 62 jne 0x76
14: e8 71 b1 76 fd callq 0xfd76b18a
19: 48 89 da mov %rbx,%rdx
1c: 48 b8 00 00 00 00 00 movabs $0xdffffc0000000000,%rax
23: fc ff df
26: 48 c1 ea 03 shr $0x3,%rdx
* 2a: 0f b6 04 02 movzbl (%rdx,%rax,1),%eax <-- trapping instruction
2e: 48 89 da mov %rbx,%rdx
31: 83 e2 07 and $0x7,%edx
34: 38 d0 cmp %dl,%al
36: 7f 08 jg 0x40
38: 84 c0 test %al,%al
3a: 0f 85 e2 01 00 00 jne 0x222


---
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.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

[Tetsuo Handa]

syzbot is reporting that "vfs: parse: deal with zero length string value"
in linux-next.git broke tmpfs's mount option parsing, for tmpfs is expecting that
vfs_parse_fs_string() returning 0 implies that param.string != NULL.

The "nr_inodes" parameter for tmpfs is interpreted as "nr_inodes=$integer", but
the addition of

if (!v_size) {
param.string = NULL;
param.type = fs_value_is_empty;
} else {

to vfs_parse_fs_string() and

if (param->type == fs_value_is_empty)
return 0;

to fs_param_is_string() broke expectation by tmpfs.

Parsing an fs string that has zero length should result in the parameter
being set to NULL so that downstream processing handles it correctly.

is wrong and

Parsing an fs string that has zero length should result in invalid argument
error so that downstream processing does not dereference NULL param.string
field.

is correct for the "nr_inodes" parameter.



How do we want to fix?
Should we add param.string != NULL checks into the downstream callers (like
Hawkins Jiawei did for https://syzkaller.appspot.com/bug?extid=a3e6acd85ded5c16a709 ) ?
Or should we add

if (!*param.string)
param.string = NULL;

rewriting into downstream callers which expect

For example, the proc mount table processing should print "(none)" in this
case to preserve mount record field count, but if the value points to the
NULL string this doesn't happen.

behavior?

[Hugh Dickins]

I've given it no thought at all: I was hoping, as Al suggests in
https://lore.kernel.org/lkml/Y1VwdUYGvDE4yUoI@ZenIV/
that the breaking commit would soon be reverted, and Ian think again.

Hugh

[Ian Kent]

Except that I didn't see the message so I haven't given it extra thought

myself either, oops!


akpm and Theodore also had concerns about the series.


The other way to fix this is to modify the proc processing to check

for zero length strings and check for any other places that need

fixing. But that means handling it downstream for individual allocated

empty string instances rather than at the source which is what I was

hoping to avoid.


But clearly there are hard to find assumptions in code that I've missed

and this instance isn't the first case of it so may be we have to drop

the series.


I can't think of any other way to do this without requiring NULL be

handled, does anyone have any thoughts to offer?


Ian

[Ian Kent]

On 24/10/22 02:50, Hugh Dickins wrote:

> On Sun, 23 Oct 2022, Tetsuo Handa wrote:
>
>> syzbot is reporting that "vfs: parse: deal with zero length string value"
>> in linux-next.git broke tmpfs's mount option parsing, for tmpfs is expecting that
>> vfs_parse_fs_string() returning 0 implies that param.string != NULL.
>>
>> The "nr_inodes" parameter for tmpfs is interpreted as "nr_inodes=$integer", but
>> the addition of
>>
>> if (!v_size) {
>> param.string = NULL;
>> param.type = fs_value_is_empty;
>> } else {
>>
>> to vfs_parse_fs_string() and
>>
>> if (param->type == fs_value_is_empty)
>> return 0;
>>
>> to fs_param_is_string() broke expectation by tmpfs.
>>
>> Parsing an fs string that has zero length should result in the parameter
>> being set to NULL so that downstream processing handles it correctly.
>>
>> is wrong and
>>
>> Parsing an fs string that has zero length should result in invalid argument
>> error so that downstream processing does not dereference NULL param.string
>> field.

It's not quite as simple at that.


Not allowing a zero length string will break cases where mount "source"

can be empty.


Maybe parsing of "source" would be better handled separately, rather than

with options handling code, it is slightly different ... mmm ... I'll check

the reported cases ...


Ian

[syzbot]

Auto-closing this bug as obsolete.
No recent activity, existing reproducers are no longer triggering the issue.