Messages in this thread Patch in this message | | | Date | Fri, 16 Jul 2021 15:24:40 +0300 | From | Pavel Skripkin <> | Subject | Re: [syzbot] UBSAN: shift-out-of-bounds in profile_init |
| |
On Wed, 14 Jul 2021 05:47:21 -0700 syzbot <syzbot+e68c89a9510c159d9684@syzkaller.appspotmail.com> wrote:
> Hello, > > syzbot found the following issue on: > > HEAD commit: 3dbdb38e Merge branch 'for-5.14' of > git://git.kernel.org/p.. git tree: upstream > console output: > https://syzkaller.appspot.com/x/log.txt?x=11342328300000 kernel > config: https://syzkaller.appspot.com/x/.config?x=a1fcf15a09815757 > dashboard link: > https://syzkaller.appspot.com/bug?extid=e68c89a9510c159d9684 syz > repro: > https://syzkaller.appspot.com/x/repro.syz?x=149a96d2300000 C > reproducer: https://syzkaller.appspot.com/x/repro.c?x=114e5bc4300000 > > IMPORTANT: if you fix the issue, please add the following tag to the > commit: Reported-by: > syzbot+e68c89a9510c159d9684@syzkaller.appspotmail.com > > kernel profiling enabled (shift: 1000000) > ================================================================================ > UBSAN: shift-out-of-bounds in kernel/profile.c:110:31 > shift exponent 1000000 is too large for 64-bit type 'long int' > CPU: 0 PID: 8453 Comm: syz-executor969 Tainted: G W > 5.13.0-syzkaller #0 Hardware name: Google Google Compute > Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: > __dump_stack lib/dump_stack.c:79 [inline] > dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:96 > ubsan_epilogue+0xb/0x5a lib/ubsan.c:148 > __ubsan_handle_shift_out_of_bounds.cold+0xb1/0x181 lib/ubsan.c:327 > profile_init+0xfc/0x110 kernel/profile.c:110 > profiling_store+0x5e/0xd0 kernel/ksysfs.c:80 > kobj_attr_store+0x50/0x80 lib/kobject.c:856 > sysfs_kf_write+0x110/0x160 fs/sysfs/file.c:139
I believe, something like this should fix it:
diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c index 35859da8bd4f..ca075d9f671a 100644 --- a/kernel/ksysfs.c +++ b/kernel/ksysfs.c @@ -76,7 +76,9 @@ static ssize_t profiling_store(struct kobject *kobj, * has a ton of callers and is not const. It is * easiest to cast it away here. */ - profile_setup((char *)buf); + ret = profile_setup((char *)buf); + if (!ret) + return -EINVAL; ret = profile_init(); if (ret) return ret; diff --git a/kernel/profile.c b/kernel/profile.c index c2ebddb5e974..5c61677030f4 100644 --- a/kernel/profile.c +++ b/kernel/profile.c @@ -42,6 +42,7 @@ struct profile_hit { static atomic_t *prof_buffer; static unsigned long prof_len, prof_shift; +#define MAX_PROF_SHIFT sizeof(prof_shift) * 8 int prof_on __read_mostly; EXPORT_SYMBOL_GPL(prof_on); @@ -66,8 +67,11 @@ int profile_setup(char *str) prof_on = SLEEP_PROFILING; if (str[strlen(sleepstr)] == ',') str += strlen(sleepstr) + 1; - if (get_option(&str, &par)) + if (get_option(&str, &par)) { + if (par >= MAX_PROF_SHIFT) + return 0; prof_shift = par; + } pr_info("kernel sleep profiling enabled (shift: %ld)\n", prof_shift); #else @@ -77,19 +81,27 @@ int profile_setup(char *str) prof_on = SCHED_PROFILING; if (str[strlen(schedstr)] == ',') str += strlen(schedstr) + 1; - if (get_option(&str, &par)) + if (get_option(&str, &par)) { + if (par >= MAX_PROF_SHIFT) + return 0; prof_shift = par; + } pr_info("kernel schedule profiling enabled (shift: %ld)\n", prof_shift); } else if (!strncmp(str, kvmstr, strlen(kvmstr))) { prof_on = KVM_PROFILING; if (str[strlen(kvmstr)] == ',') str += strlen(kvmstr) + 1; - if (get_option(&str, &par)) + if (get_option(&str, &par)) { + if (par >= MAX_PROF_SHIFT) + return 0; prof_shift = par; + } pr_info("kernel KVM profiling enabled (shift: %ld)\n", prof_shift); } else if (get_option(&str, &par)) { + if (par >= MAX_PROF_SHIFT) + return 0; prof_shift = par; prof_on = CPU_PROFILING; pr_info("kernel profiling enabled (shift: %ld)\n",
But this function can be called not only from sysfs and I can't understand will my patch break something or not. And, I think, error message is needed somewhere here to inform callers about wrong shift value.
Thoughts?
-- With regards, Pavel Skripkin
| |