| Message ID | 20230607223755.1610-2-richard@nod.at (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | Integer overflows while scanning for integers | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Thu, Jun 08, 2023 at 12:37:55AM +0200, Richard Weinberger wrote: > The scanf function family has no way to indicate overflows > while scanning. As consequence users of these function have to make > sure their input cannot cause an overflow. > Since this is not always the case add WARN_ON_ONCE() guards to > trigger a warning upon an overflow. ... > if (prefix_chars < max_chars) { > rv = _parse_integer_limit(cp, base, &result, max_chars - prefix_chars); > + WARN_ON_ONCE(rv & KSTRTOX_OVERFLOW); This seems incorrect. simple_strto*() are okay to overflow. It's by design. > /* FIXME */ ...and that's why this one is here. > cp += (rv & ~KSTRTOX_OVERFLOW); > } else {
----- Ursprüngliche Mail ----- > Von: "Andy Shevchenko" <andriy.shevchenko@linux.intel.com> >> if (prefix_chars < max_chars) { >> rv = _parse_integer_limit(cp, base, &result, max_chars - prefix_chars); >> + WARN_ON_ONCE(rv & KSTRTOX_OVERFLOW); > > This seems incorrect. simple_strto*() are okay to overflow. It's by design. Is this design decision also known to all users of scanf functions in the kernel? Thanks, //richard
On Thu, Jun 08, 2023 at 06:14:33PM +0200, Richard Weinberger wrote: > ----- Ursprüngliche Mail ----- > > Von: "Andy Shevchenko" <andriy.shevchenko@linux.intel.com> > >> if (prefix_chars < max_chars) { > >> rv = _parse_integer_limit(cp, base, &result, max_chars - prefix_chars); > >> + WARN_ON_ONCE(rv & KSTRTOX_OVERFLOW); > > > > This seems incorrect. simple_strto*() are okay to overflow. It's by design. > > Is this design decision also known to all users of scanf functions in the kernel? We have test_scanf.c. Does it miss any test cases? Please add them!
hi Richard Weinberger,
we noticed the new warning added in this patch was hit.
we know this commit should not be the real root cause, just send out this
report FYI.
Hello,
kernel test robot noticed "WARNING:at_lib/vsprintf.c:#vsscanf" on:
commit: 5f4287fc4655b77bfb9012a7a0ed630d65d01695 ("[RFC PATCH 1/1] vsprintf: Warn on integer scanning overflows")
url: https://github.com/intel-lab-lkp/linux/commits/Richard-Weinberger/vsprintf-Warn-on-integer-scanning-overflows/20230608-064044
base: https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git for-next/pstore
patch link: https://lore.kernel.org/all/20230607223755.1610-2-richard@nod.at/
patch subject: [RFC PATCH 1/1] vsprintf: Warn on integer scanning overflows
in testcase: kunit
version:
with following parameters:
group: group-01
compiler: gcc-12
test machine: 16 threads 1 sockets Intel(R) Xeon(R) CPU D-1541 @ 2.10GHz (Broadwell-DE) with 48G memory
(please refer to attached dmesg/kmsg for entire log/backtrace)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202306121011.2487b7a9-oliver.sang@intel.com
[ 105.244690][ T819] ------------[ cut here ]------------
[ 105.253891][ T819] WARNING: CPU: 11 PID: 819 at lib/vsprintf.c:3701 vsscanf (lib/vsprintf.c:3701 (discriminator 1))
[ 105.272240][ T819] Modules linked in: test_scanf(N+) intel_rapl_msr intel_rapl_common sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel kvm irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel sha512_ssse3 btrfs blake2b_generic xor raid6_pq libcrc32c crc32c_intel rapl sd_mod t10_pi crc64_rocksoft_generic crc64_rocksoft crc64 sg ipmi_ssif intel_cstate ast drm_shmem_helper ahci drm_kms_helper libahci acpi_ipmi intel_uncore mei_me syscopyarea libata ipmi_si ioatdma joydev sysfillrect gpio_ich intel_pch_thermal sysimgblt mei mxm_wmi ipmi_devintf dca ipmi_msghandler wmi acpi_pad drm fuse ip_tables [last unloaded: test_static_key_base]
[ 105.380736][ T819] CPU: 11 PID: 819 Comm: modprobe Tainted: G S N 6.4.0-rc1-00002-g5f4287fc4655 #1
[ 105.392450][ T819] Hardware name: Supermicro SYS-5018D-FN4T/X10SDV-8C-TLN4F, BIOS 1.1 03/02/2016
[ 105.402710][ T819] RIP: 0010:vsscanf (lib/vsprintf.c:3701 (discriminator 1))
[ 105.409007][ T819] Code: 4c 89 ef e8 4c ef eb fd 48 0f ba b4 24 a0 00 00 00 00 44 8b 4c 24 10 e9 7e ef ff ff 0f 0b e9 dd fc ff ff 0f 0b e9 d6 fc ff ff <0f> 0b e9 5b f8 ff ff 0f 0b e9 1b f9 ff ff 0f 0b e9 14 f9 ff ff 0f
All code
========
0: 4c 89 ef mov %r13,%rdi
3: e8 4c ef eb fd callq 0xfffffffffdebef54
8: 48 0f ba b4 24 a0 00 btrq $0x0,0xa0(%rsp)
f: 00 00 00
12: 44 8b 4c 24 10 mov 0x10(%rsp),%r9d
17: e9 7e ef ff ff jmpq 0xffffffffffffef9a
1c: 0f 0b ud2
1e: e9 dd fc ff ff jmpq 0xfffffffffffffd00
23: 0f 0b ud2
25: e9 d6 fc ff ff jmpq 0xfffffffffffffd00
2a:* 0f 0b ud2 <-- trapping instruction
2c: e9 5b f8 ff ff jmpq 0xfffffffffffff88c
31: 0f 0b ud2
33: e9 1b f9 ff ff jmpq 0xfffffffffffff953
38: 0f 0b ud2
3a: e9 14 f9 ff ff jmpq 0xfffffffffffff953
3f: 0f .byte 0xf
Code starting with the faulting instruction
===========================================
0: 0f 0b ud2
2: e9 5b f8 ff ff jmpq 0xfffffffffffff862
7: 0f 0b ud2
9: e9 1b f9 ff ff jmpq 0xfffffffffffff929
e: 0f 0b ud2
10: e9 14 f9 ff ff jmpq 0xfffffffffffff929
15: 0f .byte 0xf
[ 105.431369][ T819] RSP: 0018:ffffc9000120f730 EFLAGS: 00010206
[ 105.438770][ T819] RAX: 00000000ffffffff RBX: 00000000ffffffff RCX: 000000000000000f
[ 105.448085][ T819] RDX: 00000000ffffffff RSI: ffffc9000120f6c0 RDI: 00000000fffffff0
[ 105.457422][ T819] RBP: 1ffff92000241eee R08: ffff8881f4af5fff R09: 00000000ffffffff
[ 105.466692][ T819] R10: 00000000000000ff R11: 0000000000000010 R12: dffffc0000000000
[ 105.475991][ T819] R13: 0000000000000000 R14: ffffc9000120f8d0 R15: ffffffffc0a2ca42
[ 105.485326][ T819] FS: 00007f4bfafd0540(0000) GS:ffff888ab9b80000(0000) knlGS:0000000000000000
[ 105.495578][ T819] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 105.503457][ T819] CR2: 00005645746ea122 CR3: 000000016b904002 CR4: 00000000003706e0
[ 105.512757][ T819] DR0: ffffffff8635204c DR1: ffffffff8635204d DR2: ffffffff8635204e
[ 105.522075][ T819] DR3: ffffffff8635204f DR6: 00000000fffe0ff0 DR7: 0000000000000600
[ 105.531423][ T819] Call Trace:
[ 105.536017][ T819] <TASK>
[ 105.540310][ T819] ? simple_strtol (lib/vsprintf.c:3433)
[ 105.546195][ T819] ? _raw_spin_lock_irqsave (arch/x86/include/asm/atomic.h:202 include/linux/atomic/atomic-instrumented.h:543 include/asm-generic/qspinlock.h:111 include/linux/spinlock.h:186 include/linux/spinlock_api_smp.h:111 kernel/locking/spinlock.c:162)
[ 105.552848][ T819] ? _raw_read_unlock_irqrestore (kernel/locking/spinlock.c:161)
[ 105.559938][ T819] ? vsnprintf (lib/vsprintf.c:2768)
[ 105.565718][ T819] ? check_ushort (lib/test_scanf.c:120) test_scanf
[ 105.572801][ T819] _test (lib/test_scanf.c:44) test_scanf
[ 105.579005][ T819] ? check_ull (lib/test_scanf.c:33) test_scanf
[ 105.585851][ T819] ? kasan_save_stack (mm/kasan/common.c:46)
[ 105.591946][ T819] ? snprintf (lib/vsprintf.c:2948)
[ 105.597318][ T819] numbers_simple (lib/test_scanf.c:242 (discriminator 8)) test_scanf
[ 105.604512][ T819] ? _test (lib/test_scanf.c:218) test_scanf
[ 105.610909][ T819] selftest (lib/test_scanf.c:771 lib/test_scanf.c:800) test_scanf
[ 105.617312][ T819] ? selftest (lib/test_scanf.c:811) test_scanf
[ 105.623967][ T819] test_scanf_init (lib/test_scanf.c:811) test_scanf
[ 105.630968][ T819] do_one_initcall (init/main.c:1246)
[ 105.636828][ T819] ? trace_event_raw_event_initcall_level (init/main.c:1237)
[ 105.644758][ T819] ? kasan_unpoison (mm/kasan/shadow.c:160 mm/kasan/shadow.c:194)
[ 105.650593][ T819] do_init_module (kernel/module/main.c:2529)
[ 105.656423][ T819] load_module (kernel/module/main.c:2980)
[ 105.662145][ T819] ? post_relocation (kernel/module/main.c:2829)
[ 105.668227][ T819] ? __x64_sys_fspick (fs/kernel_read_file.c:38)
[ 105.674383][ T819] ? __do_sys_finit_module (kernel/module/main.c:3099)
[ 105.680880][ T819] __do_sys_finit_module (kernel/module/main.c:3099)
[ 105.687189][ T819] ? __ia32_sys_init_module (kernel/module/main.c:3061)
[ 105.693654][ T819] ? randomize_page (mm/util.c:533)
[ 105.699366][ T819] ? ksys_mmap_pgoff (mm/mmap.c:1445)
[ 105.705319][ T819] ? __fdget_pos (arch/x86/include/asm/atomic64_64.h:22 include/linux/atomic/atomic-long.h:29 include/linux/atomic/atomic-instrumented.h:1310 fs/file.c:1045)
[ 105.710712][ T819] do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
[ 105.716060][ T819] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
[ 105.722870][ T819] RIP: 0033:0x7f4bfb0f19b9
[ 105.728156][ T819] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 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 8b 0d a7 54 0c 00 f7 d8 64 89 01 48
All code
========
0: 00 c3 add %al,%bl
2: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
9: 00 00 00
c: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
11: 48 89 f8 mov %rdi,%rax
14: 48 89 f7 mov %rsi,%rdi
17: 48 89 d6 mov %rdx,%rsi
1a: 48 89 ca mov %rcx,%rdx
1d: 4d 89 c2 mov %r8,%r10
20: 4d 89 c8 mov %r9,%r8
23: 4c 8b 4c 24 08 mov 0x8(%rsp),%r9
28: 0f 05 syscall
2a:* 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax <-- trapping instruction
30: 73 01 jae 0x33
32: c3 retq
33: 48 8b 0d a7 54 0c 00 mov 0xc54a7(%rip),%rcx # 0xc54e1
3a: f7 d8 neg %eax
3c: 64 89 01 mov %eax,%fs:(%rcx)
3f: 48 rex.W
Code starting with the faulting instruction
===========================================
0: 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax
6: 73 01 jae 0x9
8: c3 retq
9: 48 8b 0d a7 54 0c 00 mov 0xc54a7(%rip),%rcx # 0xc54b7
10: f7 d8 neg %eax
12: 64 89 01 mov %eax,%fs:(%rcx)
15: 48 rex.W
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
sudo bin/lkp install job.yaml # job file is attached in this email
bin/lkp split-job --compatible job.yaml # generate the yaml file for lkp run
sudo bin/lkp run generated-yaml-file
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 40f560959b169..3d8d751306cdc 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -70,6 +70,7 @@ static noinline unsigned long long simple_strntoull(const char *startp, size_t m prefix_chars = cp - startp; if (prefix_chars < max_chars) { rv = _parse_integer_limit(cp, base, &result, max_chars - prefix_chars); + WARN_ON_ONCE(rv & KSTRTOX_OVERFLOW); /* FIXME */ cp += (rv & ~KSTRTOX_OVERFLOW); } else { @@ -3657,22 +3658,34 @@ int vsscanf(const char *buf, const char *fmt, va_list args) switch (qualifier) { case 'H': /* that's 'hh' in format */ - if (is_sign) + if (is_sign) { + WARN_ON_ONCE(val.s > 127); + WARN_ON_ONCE(val.s < -128); *va_arg(args, signed char *) = val.s; - else + } else { + WARN_ON_ONCE(val.u > 255); *va_arg(args, unsigned char *) = val.u; + } break; case 'h': - if (is_sign) + if (is_sign) { + WARN_ON_ONCE(val.s > SHRT_MAX); + WARN_ON_ONCE(val.s < SHRT_MIN); *va_arg(args, short *) = val.s; - else + } else { + WARN_ON_ONCE(val.u > USHRT_MAX); *va_arg(args, unsigned short *) = val.u; + } break; case 'l': - if (is_sign) + if (is_sign) { + WARN_ON_ONCE(val.s > LONG_MAX); + WARN_ON_ONCE(val.s < LONG_MIN); *va_arg(args, long *) = val.s; - else + } else { + WARN_ON_ONCE(val.u > ULONG_MAX); *va_arg(args, unsigned long *) = val.u; + } break; case 'L': if (is_sign) @@ -3684,10 +3697,14 @@ int vsscanf(const char *buf, const char *fmt, va_list args) *va_arg(args, size_t *) = val.u; break; default: - if (is_sign) + if (is_sign) { + WARN_ON_ONCE(val.s > INT_MAX); + WARN_ON_ONCE(val.s < INT_MIN); *va_arg(args, int *) = val.s; - else + } else { + WARN_ON_ONCE(val.u > UINT_MAX); *va_arg(args, unsigned int *) = val.u; + } break; } num++;
The scanf function family has no way to indicate overflows while scanning. As consequence users of these function have to make sure their input cannot cause an overflow. Since this is not always the case add WARN_ON_ONCE() guards to trigger a warning upon an overflow. Signed-off-by: Richard Weinberger <richard@nod.at> --- lib/vsprintf.c | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-)