* [PATCH] profiling: fix shift-out-of-bounds in profile_setup
@ 2021-07-16 19:04 Pavel Skripkin
2021-08-08 11:21 ` Tetsuo Handa
0 siblings, 1 reply; 7+ messages in thread
From: Pavel Skripkin @ 2021-07-16 19:04 UTC (permalink / raw)
To: penguin-kernel, rostedt, tglx
Cc: linux-kernel, Pavel Skripkin, syzbot+e68c89a9510c159d9684, Tetsuo Handa
Syzbot reported shift-out-of-bounds bug in profile_init().
The problem was in incorrect prof_shift. Since prof_shift value comes from
userspace we need to check this value to avoid too big shift.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-and-tested-by: syzbot+e68c89a9510c159d9684@syzkaller.appspotmail.com
Suggested-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
kernel/profile.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/kernel/profile.c b/kernel/profile.c
index c2ebddb5e974..c905931e3c3b 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);
@@ -67,7 +68,7 @@ int profile_setup(char *str)
if (str[strlen(sleepstr)] == ',')
str += strlen(sleepstr) + 1;
if (get_option(&str, &par))
- prof_shift = par;
+ prof_shift = clamp(par, 0, (int) MAX_PROF_SHIFT - 1);
pr_info("kernel sleep profiling enabled (shift: %ld)\n",
prof_shift);
#else
@@ -78,7 +79,7 @@ int profile_setup(char *str)
if (str[strlen(schedstr)] == ',')
str += strlen(schedstr) + 1;
if (get_option(&str, &par))
- prof_shift = par;
+ prof_shift = clamp(par, 0, (int) MAX_PROF_SHIFT - 1);
pr_info("kernel schedule profiling enabled (shift: %ld)\n",
prof_shift);
} else if (!strncmp(str, kvmstr, strlen(kvmstr))) {
@@ -86,11 +87,11 @@ int profile_setup(char *str)
if (str[strlen(kvmstr)] == ',')
str += strlen(kvmstr) + 1;
if (get_option(&str, &par))
- prof_shift = par;
+ prof_shift = clamp(par, 0, (int) MAX_PROF_SHIFT - 1);
pr_info("kernel KVM profiling enabled (shift: %ld)\n",
prof_shift);
} else if (get_option(&str, &par)) {
- prof_shift = par;
+ prof_shift = clamp(par, 0, (int) MAX_PROF_SHIFT - 1);
prof_on = CPU_PROFILING;
pr_info("kernel profiling enabled (shift: %ld)\n",
prof_shift);
--
2.32.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] profiling: fix shift-out-of-bounds in profile_setup 2021-07-16 19:04 [PATCH] profiling: fix shift-out-of-bounds in profile_setup Pavel Skripkin @ 2021-08-08 11:21 ` Tetsuo Handa 2021-08-13 10:56 ` Pavel Skripkin 0 siblings, 1 reply; 7+ messages in thread From: Tetsuo Handa @ 2021-08-08 11:21 UTC (permalink / raw) To: Pavel Skripkin, rostedt, tglx; +Cc: linux-kernel, syzbot+e68c89a9510c159d9684 On 2021/07/17 4:04, Pavel Skripkin wrote: > Syzbot reported shift-out-of-bounds bug in profile_init(). > The problem was in incorrect prof_shift. Since prof_shift value comes from > userspace we need to check this value to avoid too big shift. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Reported-and-tested-by: syzbot+e68c89a9510c159d9684@syzkaller.appspotmail.com > Suggested-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> > --- > kernel/profile.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/kernel/profile.c b/kernel/profile.c > index c2ebddb5e974..c905931e3c3b 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) I came to think that we should directly use BITS_PER_LONG, for the integer value which is subjected to shift operation is e.g. (_etext - _stext) part of prof_len = (_etext - _stext) >> prof_shift; in profile_init(). Since "unsigned char" will be sufficient for holding BITS_PER_LONG - 1, defining MAX_PROF_SHIFT based on size of prof_shift is incorrect. Also, there is unsigned int sample_step = 1 << prof_shift; in read_profile(). This may result in shift-out-of-bounds on BITS_PER_LONG == 64 architecture. Shouldn't this variable changed from "unsigned int" to "unsigned long" and use 1UL instead of 1 ? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] profiling: fix shift-out-of-bounds in profile_setup 2021-08-08 11:21 ` Tetsuo Handa @ 2021-08-13 10:56 ` Pavel Skripkin 2021-08-13 13:27 ` Tetsuo Handa 0 siblings, 1 reply; 7+ messages in thread From: Pavel Skripkin @ 2021-08-13 10:56 UTC (permalink / raw) To: Tetsuo Handa, rostedt, tglx; +Cc: linux-kernel, syzbot+e68c89a9510c159d9684 On 8/8/21 2:21 PM, Tetsuo Handa wrote: > On 2021/07/17 4:04, Pavel Skripkin wrote: >> Syzbot reported shift-out-of-bounds bug in profile_init(). >> The problem was in incorrect prof_shift. Since prof_shift value comes from >> userspace we need to check this value to avoid too big shift. >> >> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") >> Reported-and-tested-by: syzbot+e68c89a9510c159d9684@syzkaller.appspotmail.com >> Suggested-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> >> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> >> --- >> kernel/profile.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/kernel/profile.c b/kernel/profile.c >> index c2ebddb5e974..c905931e3c3b 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) > > I came to think that we should directly use BITS_PER_LONG, for > the integer value which is subjected to shift operation is e.g. > > (_etext - _stext) > > part of > > prof_len = (_etext - _stext) >> prof_shift; > > in profile_init(). > > Since "unsigned char" will be sufficient for holding BITS_PER_LONG - 1, > defining MAX_PROF_SHIFT based on size of prof_shift is incorrect. > I don't get it, sorry. Do you mean, that #define MAX_PROF_SHIFT BITS_PER_LONG is better solution here? And as I understand we can change prof_shift type from "unsigned long" to "unsigned short", rigth? > Also, there is > > unsigned int sample_step = 1 << prof_shift; > > in read_profile(). This may result in shift-out-of-bounds on BITS_PER_LONG == 64 > architecture. Shouldn't this variable changed from "unsigned int" to "unsigned long" > and use 1UL instead of 1 ? > Yep, it's necessary. Will do it in v2 With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] profiling: fix shift-out-of-bounds in profile_setup 2021-08-13 10:56 ` Pavel Skripkin @ 2021-08-13 13:27 ` Tetsuo Handa 2021-08-13 14:00 ` [PATCH v2] profiling: fix shift-out-of-bounds bugs Pavel Skripkin 0 siblings, 1 reply; 7+ messages in thread From: Tetsuo Handa @ 2021-08-13 13:27 UTC (permalink / raw) To: Pavel Skripkin, rostedt, tglx; +Cc: linux-kernel, syzbot+e68c89a9510c159d9684 On 2021/08/13 19:56, Pavel Skripkin wrote: > I don't get it, sorry. Do you mean, that > > #define MAX_PROF_SHIFT BITS_PER_LONG > > is better solution here? Yes, but I feel we don't need to define MAX_PROF_SHIFT because the type of the integer value which is subjected to shift operation will be always "unsigned long" and will unlikely change in the future. > And as I understand we can change prof_shift type from "unsigned long" to "unsigned short", rigth? Yes, "unsigned int" or "unsigned short int", or even "u8" (I don't know whether compilers generate bad code if "u8" is used). At least, since get_option() stores result into "int", receiving via "unsigned int" will be enough. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] profiling: fix shift-out-of-bounds bugs 2021-08-13 13:27 ` Tetsuo Handa @ 2021-08-13 14:00 ` Pavel Skripkin 2021-08-19 20:46 ` Steven Rostedt 0 siblings, 1 reply; 7+ messages in thread From: Pavel Skripkin @ 2021-08-13 14:00 UTC (permalink / raw) To: rostedt, penguin-kernel, tglx Cc: linux-kernel, Pavel Skripkin, syzbot+e68c89a9510c159d9684, Tetsuo Handa Syzbot reported shift-out-of-bounds bug in profile_init(). The problem was in incorrect prof_shift. Since prof_shift value comes from userspace we need to clamp this value into [0, BITS_PER_LONG -1] boundaries. Second possible shiht-out-of-bounds was found by Tetsuo: sample_step local variable in read_profile() had "unsigned int" type, but prof_shift allows to make a BITS_PER_LONG shift. So, to prevent possible shiht-out-of-bounds sample_step type was changed to "unsigned long". Also, "unsigned short int" will be sufficient for storing [0, BITS_PER_LONG] value, that's why there is no need for "unsigned long" prof_shift. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Reported-and-tested-by: syzbot+e68c89a9510c159d9684@syzkaller.appspotmail.com Suggested-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> --- Changes in v2: 1. Fixed possible shiht-out-of-bounds in read_profile() (Reported by Tetsuo) 2. Changed prof_shift type from "unsigned long" to "unsigned short int" --- kernel/profile.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/kernel/profile.c b/kernel/profile.c index c2ebddb5e974..eb9c7f0f5ac5 100644 --- a/kernel/profile.c +++ b/kernel/profile.c @@ -41,7 +41,8 @@ struct profile_hit { #define NR_PROFILE_GRP (NR_PROFILE_HIT/PROFILE_GRPSZ) static atomic_t *prof_buffer; -static unsigned long prof_len, prof_shift; +static unsigned long prof_len; +static unsigned short int prof_shift; int prof_on __read_mostly; EXPORT_SYMBOL_GPL(prof_on); @@ -67,8 +68,8 @@ int profile_setup(char *str) if (str[strlen(sleepstr)] == ',') str += strlen(sleepstr) + 1; if (get_option(&str, &par)) - prof_shift = par; - pr_info("kernel sleep profiling enabled (shift: %ld)\n", + prof_shift = clamp(par, 0, BITS_PER_LONG - 1); + pr_info("kernel sleep profiling enabled (shift: %u)\n", prof_shift); #else pr_warn("kernel sleep profiling requires CONFIG_SCHEDSTATS\n"); @@ -78,21 +79,21 @@ int profile_setup(char *str) if (str[strlen(schedstr)] == ',') str += strlen(schedstr) + 1; if (get_option(&str, &par)) - prof_shift = par; - pr_info("kernel schedule profiling enabled (shift: %ld)\n", + prof_shift = clamp(par, 0, BITS_PER_LONG - 1); + pr_info("kernel schedule profiling enabled (shift: %u)\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)) - prof_shift = par; - pr_info("kernel KVM profiling enabled (shift: %ld)\n", + prof_shift = clamp(par, 0, BITS_PER_LONG - 1); + pr_info("kernel KVM profiling enabled (shift: %u)\n", prof_shift); } else if (get_option(&str, &par)) { - prof_shift = par; + prof_shift = clamp(par, 0, BITS_PER_LONG - 1); prof_on = CPU_PROFILING; - pr_info("kernel profiling enabled (shift: %ld)\n", + pr_info("kernel profiling enabled (shift: %u)\n", prof_shift); } return 1; @@ -468,7 +469,7 @@ read_profile(struct file *file, char __user *buf, size_t count, loff_t *ppos) unsigned long p = *ppos; ssize_t read; char *pnt; - unsigned int sample_step = 1 << prof_shift; + unsigned long sample_step = 1UL << prof_shift; profile_flip_buffers(); if (p >= (prof_len+1)*sizeof(unsigned int)) -- 2.32.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] profiling: fix shift-out-of-bounds bugs 2021-08-13 14:00 ` [PATCH v2] profiling: fix shift-out-of-bounds bugs Pavel Skripkin @ 2021-08-19 20:46 ` Steven Rostedt 2021-08-29 1:57 ` Tetsuo Handa 0 siblings, 1 reply; 7+ messages in thread From: Steven Rostedt @ 2021-08-19 20:46 UTC (permalink / raw) To: Pavel Skripkin Cc: penguin-kernel, tglx, linux-kernel, syzbot+e68c89a9510c159d9684, Andrew Morton Who's taking this patch? Or should Andrew just take it through his tree? -- Steve On Fri, 13 Aug 2021 17:00:22 +0300 Pavel Skripkin <paskripkin@gmail.com> wrote: > Syzbot reported shift-out-of-bounds bug in profile_init(). > The problem was in incorrect prof_shift. Since prof_shift value comes from > userspace we need to clamp this value into [0, BITS_PER_LONG -1] > boundaries. > > Second possible shiht-out-of-bounds was found by Tetsuo: > sample_step local variable in read_profile() had "unsigned int" type, > but prof_shift allows to make a BITS_PER_LONG shift. So, to prevent > possible shiht-out-of-bounds sample_step type was changed to > "unsigned long". > > Also, "unsigned short int" will be sufficient for storing > [0, BITS_PER_LONG] value, that's why there is no need for > "unsigned long" prof_shift. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Reported-and-tested-by: syzbot+e68c89a9510c159d9684@syzkaller.appspotmail.com > Suggested-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> > --- > > Changes in v2: > 1. Fixed possible shiht-out-of-bounds in read_profile() > (Reported by Tetsuo) > > 2. Changed prof_shift type from "unsigned long" to > "unsigned short int" > > --- > kernel/profile.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) > > diff --git a/kernel/profile.c b/kernel/profile.c > index c2ebddb5e974..eb9c7f0f5ac5 100644 > --- a/kernel/profile.c > +++ b/kernel/profile.c > @@ -41,7 +41,8 @@ struct profile_hit { > #define NR_PROFILE_GRP (NR_PROFILE_HIT/PROFILE_GRPSZ) > > static atomic_t *prof_buffer; > -static unsigned long prof_len, prof_shift; > +static unsigned long prof_len; > +static unsigned short int prof_shift; > > int prof_on __read_mostly; > EXPORT_SYMBOL_GPL(prof_on); > @@ -67,8 +68,8 @@ int profile_setup(char *str) > if (str[strlen(sleepstr)] == ',') > str += strlen(sleepstr) + 1; > if (get_option(&str, &par)) > - prof_shift = par; > - pr_info("kernel sleep profiling enabled (shift: %ld)\n", > + prof_shift = clamp(par, 0, BITS_PER_LONG - 1); > + pr_info("kernel sleep profiling enabled (shift: %u)\n", > prof_shift); > #else > pr_warn("kernel sleep profiling requires CONFIG_SCHEDSTATS\n"); > @@ -78,21 +79,21 @@ int profile_setup(char *str) > if (str[strlen(schedstr)] == ',') > str += strlen(schedstr) + 1; > if (get_option(&str, &par)) > - prof_shift = par; > - pr_info("kernel schedule profiling enabled (shift: %ld)\n", > + prof_shift = clamp(par, 0, BITS_PER_LONG - 1); > + pr_info("kernel schedule profiling enabled (shift: %u)\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)) > - prof_shift = par; > - pr_info("kernel KVM profiling enabled (shift: %ld)\n", > + prof_shift = clamp(par, 0, BITS_PER_LONG - 1); > + pr_info("kernel KVM profiling enabled (shift: %u)\n", > prof_shift); > } else if (get_option(&str, &par)) { > - prof_shift = par; > + prof_shift = clamp(par, 0, BITS_PER_LONG - 1); > prof_on = CPU_PROFILING; > - pr_info("kernel profiling enabled (shift: %ld)\n", > + pr_info("kernel profiling enabled (shift: %u)\n", > prof_shift); > } > return 1; > @@ -468,7 +469,7 @@ read_profile(struct file *file, char __user *buf, size_t count, loff_t *ppos) > unsigned long p = *ppos; > ssize_t read; > char *pnt; > - unsigned int sample_step = 1 << prof_shift; > + unsigned long sample_step = 1UL << prof_shift; > > profile_flip_buffers(); > if (p >= (prof_len+1)*sizeof(unsigned int)) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] profiling: fix shift-out-of-bounds bugs 2021-08-19 20:46 ` Steven Rostedt @ 2021-08-29 1:57 ` Tetsuo Handa 0 siblings, 0 replies; 7+ messages in thread From: Tetsuo Handa @ 2021-08-29 1:57 UTC (permalink / raw) To: Andrew Morton Cc: tglx, linux-kernel, syzbot+e68c89a9510c159d9684, Steven Rostedt, Pavel Skripkin Andrew, can you take this patch? On 2021/08/20 5:46, Steven Rostedt wrote: > > Who's taking this patch? Or should Andrew just take it through his tree? > > -- Steve > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-08-29 1:58 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-16 19:04 [PATCH] profiling: fix shift-out-of-bounds in profile_setup Pavel Skripkin 2021-08-08 11:21 ` Tetsuo Handa 2021-08-13 10:56 ` Pavel Skripkin 2021-08-13 13:27 ` Tetsuo Handa 2021-08-13 14:00 ` [PATCH v2] profiling: fix shift-out-of-bounds bugs Pavel Skripkin 2021-08-19 20:46 ` Steven Rostedt 2021-08-29 1:57 ` Tetsuo Handa
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).