From: Robert R. <rob...@am...> - 2011-12-19 13:47:08
|
Ingo, please pull this one fix for 3.2. Thanks, -Robert The following changes since commit dc47ce90c3a822cd7c9e9339fe4d5f61dcb26b50: Linux 3.2-rc5 (2011-12-09 15:09:32 -0800) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent Robert Richter (1): oprofile: Fix uninitialized memory access when writing to oprofilefs arch/s390/oprofile/init.c | 3 +++ drivers/oprofile/oprofile_files.c | 9 +++++++++ drivers/oprofile/oprofilefs.c | 15 ++++++++++++++- 3 files changed, 26 insertions(+), 1 deletions(-) |
From: Robert R. <rob...@am...> - 2011-12-19 13:47:06
|
If oprofilefs_ulong_from_user() is called with count equals zero, *val must be initialized. Otherwise *val is later used uninitialized as no error is returned. Alternatively oprofilefs_ulong_from_user() may not be called if !count. This patch fixes usage of oprofilefs_ulong_from_ user(). This follows write syscall implementation when count is zero: "If count is zero ... [and if] no errors are detected, 0 will be returned without causing any other effect." (man 2 write) Reported-By: Mike Waychison <mi...@go...> Cc: Andrew Morton <ak...@li...> Cc: st...@vg... Signed-off-by: Robert Richter <rob...@am...> --- arch/s390/oprofile/init.c | 3 +++ drivers/oprofile/oprofile_files.c | 9 +++++++++ drivers/oprofile/oprofilefs.c | 15 ++++++++++++++- 3 files changed, 26 insertions(+), 1 deletions(-) diff --git a/arch/s390/oprofile/init.c b/arch/s390/oprofile/init.c index 6efc18b..5d605f1 100644 --- a/arch/s390/oprofile/init.c +++ b/arch/s390/oprofile/init.c @@ -87,6 +87,9 @@ static ssize_t hwsampler_write(struct file *file, char const __user *buf, if (*offset) return -EINVAL; + if (!count) + return 0; + retval = oprofilefs_ulong_from_user(&val, buf, count); if (retval) return retval; diff --git a/drivers/oprofile/oprofile_files.c b/drivers/oprofile/oprofile_files.c index 89f6345..8265b41 100644 --- a/drivers/oprofile/oprofile_files.c +++ b/drivers/oprofile/oprofile_files.c @@ -44,6 +44,9 @@ static ssize_t timeout_write(struct file *file, char const __user *buf, if (*offset) return -EINVAL; + if (!count) + return 0; + retval = oprofilefs_ulong_from_user(&val, buf, count); if (retval) return retval; @@ -83,6 +86,9 @@ static ssize_t depth_write(struct file *file, char const __user *buf, size_t cou if (!oprofile_ops.backtrace) return -EINVAL; + if (!count) + return 0; + retval = oprofilefs_ulong_from_user(&val, buf, count); if (retval) return retval; @@ -140,6 +146,9 @@ static ssize_t enable_write(struct file *file, char const __user *buf, size_t co if (*offset) return -EINVAL; + if (!count) + return 0; + retval = oprofilefs_ulong_from_user(&val, buf, count); if (retval) return retval; diff --git a/drivers/oprofile/oprofilefs.c b/drivers/oprofile/oprofilefs.c index d0de6cc..1caf1b0 100644 --- a/drivers/oprofile/oprofilefs.c +++ b/drivers/oprofile/oprofilefs.c @@ -59,7 +59,17 @@ ssize_t oprofilefs_ulong_to_user(unsigned long val, char __user *buf, size_t cou return simple_read_from_buffer(buf, count, offset, tmpbuf, maxlen); } - +/* + * Note: oprofilefs_ulong_from_user() must be called with *val + * initialized, otherwise *val is used uninitialized if !count. This + * follows write syscall implementation when count is zero: "If count + * is zero ... [and if] no errors are detected, 0 will be returned + * without causing any other effect." (man 2 write) + * + * In case *val is a temporary variable, oprofilefs_ulong_from_user() + * may not be called if !count. This causes race conditions due to + * missing locking of *var. + */ int oprofilefs_ulong_from_user(unsigned long *val, char const __user *buf, size_t count) { char tmpbuf[TMPBUFSIZE]; @@ -98,6 +108,9 @@ static ssize_t ulong_write_file(struct file *file, char const __user *buf, size_ if (*offset) return -EINVAL; + if (!count) + return 0; + retval = oprofilefs_ulong_from_user(&value, buf, count); if (retval) return retval; -- 1.7.7 |
From: Ingo M. <mi...@el...> - 2011-12-19 14:03:01
|
* Robert Richter <rob...@am...> wrote: > Ingo, > > please pull this one fix for 3.2. > > Thanks, > > -Robert > > > > The following changes since commit dc47ce90c3a822cd7c9e9339fe4d5f61dcb26b50: > > Linux 3.2-rc5 (2011-12-09 15:09:32 -0800) > > are available in the git repository at: > git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent > > Robert Richter (1): > oprofile: Fix uninitialized memory access when writing to oprofilefs > > arch/s390/oprofile/init.c | 3 +++ > drivers/oprofile/oprofile_files.c | 9 +++++++++ > drivers/oprofile/oprofilefs.c | 15 ++++++++++++++- > 3 files changed, 26 insertions(+), 1 deletions(-) Hm, i really don't like this: + if (!count) + return 0; + retval = oprofilefs_ulong_from_user(&val, buf, count); if (retval) return retval; + if (!count) + return 0; + retval = oprofilefs_ulong_from_user(&val, buf, count); if (retval) return retval; + if (!count) + return 0; + retval = oprofilefs_ulong_from_user(&val, buf, count); if (retval) return retval; + if (!count) + return 0; + retval = oprofilefs_ulong_from_user(&val, buf, count); if (retval) return retval; + if (!count) + return 0; + retval = oprofilefs_ulong_from_user(&value, buf, count); if (retval) return retval; See the ugly and fragile pattern? This should *really* be solved via the oprofilefs_ulong_from_user() helper function, not by sprinkling the !count checks in half a dozen places ... Thanks, Ingo |
From: Robert R. <rob...@am...> - 2011-12-19 14:17:22
|
On 19.12.11 15:00:49, Ingo Molnar wrote: > + if (!count) > + return 0; > + > retval = oprofilefs_ulong_from_user(&value, buf, count); > if (retval) > return retval; > > See the ugly and fragile pattern? > > This should *really* be solved via the > oprofilefs_ulong_from_user() helper function, not by sprinkling > the !count checks in half a dozen places ... Hmm, I thought there was no way to leave the code path with count == 0 and retval. But thinking about it it would be possible with returning count or errors < 0. Will improve the patch. Thanks, -Robert -- Advanced Micro Devices, Inc. Operating System Research Center |
From: Robert R. <rob...@am...> - 2011-12-19 15:39:26
|
On 19.12.11 15:17:03, Robert Richter wrote: > On 19.12.11 15:00:49, Ingo Molnar wrote: > > > + if (!count) > > + return 0; > > + > > retval = oprofilefs_ulong_from_user(&value, buf, count); > > if (retval) > > return retval; > > > > See the ugly and fragile pattern? > > > > This should *really* be solved via the > > oprofilefs_ulong_from_user() helper function, not by sprinkling > > the !count checks in half a dozen places ... > > Hmm, I thought there was no way to leave the code path with count == 0 > and retval. But thinking about it it would be possible with returning > count or errors < 0. Will improve the patch. Ingo, see my updated version below. I am fine with appling it directly on tip/perf/urgent. Thanks, -Robert -- >From f07214bc05560f63da8404d31aa83034a16c6229 Mon Sep 17 00:00:00 2001 From: Robert Richter <rob...@am...> Date: Fri, 16 Dec 2011 15:45:31 +0100 Subject: [PATCH] oprofile: Fix uninitialized memory access when writing to oprofilefs If oprofilefs_ulong_from_user() is called with count equals zero, *val remains unchanged. Depending on the implementation it might be uninitialized. Change oprofilefs_ulong_from_user()'s interface to return count on success. Thus, we are able to return early if count equals zero which avoids using *val uninitialized. Fixing all users of oprofilefs_ulong_ from_user(). This follows write syscall implementation when count is zero: "If count is zero ... [and if] no errors are detected, 0 will be returned without causing any other effect." (man 2 write) Reported-By: Mike Waychison <mi...@go...> Cc: Andrew Morton <ak...@li...> Cc: st...@vg... Signed-off-by: Robert Richter <rob...@am...> --- arch/s390/oprofile/init.c | 2 +- drivers/oprofile/oprofile_files.c | 7 ++++--- drivers/oprofile/oprofilefs.c | 11 +++++++++-- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/arch/s390/oprofile/init.c b/arch/s390/oprofile/init.c index 6efc18b..bd58b72 100644 --- a/arch/s390/oprofile/init.c +++ b/arch/s390/oprofile/init.c @@ -88,7 +88,7 @@ static ssize_t hwsampler_write(struct file *file, char const __user *buf, return -EINVAL; retval = oprofilefs_ulong_from_user(&val, buf, count); - if (retval) + if (retval <= 0) return retval; if (oprofile_started) diff --git a/drivers/oprofile/oprofile_files.c b/drivers/oprofile/oprofile_files.c index 89f6345..84a208d 100644 --- a/drivers/oprofile/oprofile_files.c +++ b/drivers/oprofile/oprofile_files.c @@ -45,7 +45,7 @@ static ssize_t timeout_write(struct file *file, char const __user *buf, return -EINVAL; retval = oprofilefs_ulong_from_user(&val, buf, count); - if (retval) + if (retval <= 0) return retval; retval = oprofile_set_timeout(val); @@ -84,7 +84,7 @@ static ssize_t depth_write(struct file *file, char const __user *buf, size_t cou return -EINVAL; retval = oprofilefs_ulong_from_user(&val, buf, count); - if (retval) + if (retval <= 0) return retval; retval = oprofile_set_ulong(&oprofile_backtrace_depth, val); @@ -141,9 +141,10 @@ static ssize_t enable_write(struct file *file, char const __user *buf, size_t co return -EINVAL; retval = oprofilefs_ulong_from_user(&val, buf, count); - if (retval) + if (retval <= 0) return retval; + retval = 0; if (val) retval = oprofile_start(); else diff --git a/drivers/oprofile/oprofilefs.c b/drivers/oprofile/oprofilefs.c index d0de6cc..2f0aa0f 100644 --- a/drivers/oprofile/oprofilefs.c +++ b/drivers/oprofile/oprofilefs.c @@ -60,6 +60,13 @@ ssize_t oprofilefs_ulong_to_user(unsigned long val, char __user *buf, size_t cou } +/* + * Note: If oprofilefs_ulong_from_user() returns 0, then *val remains + * unchanged and might be uninitialized. This follows write syscall + * implementation when count is zero: "If count is zero ... [and if] + * no errors are detected, 0 will be returned without causing any + * other effect." (man 2 write) + */ int oprofilefs_ulong_from_user(unsigned long *val, char const __user *buf, size_t count) { char tmpbuf[TMPBUFSIZE]; @@ -79,7 +86,7 @@ int oprofilefs_ulong_from_user(unsigned long *val, char const __user *buf, size_ raw_spin_lock_irqsave(&oprofilefs_lock, flags); *val = simple_strtoul(tmpbuf, NULL, 0); raw_spin_unlock_irqrestore(&oprofilefs_lock, flags); - return 0; + return count; } @@ -99,7 +106,7 @@ static ssize_t ulong_write_file(struct file *file, char const __user *buf, size_ return -EINVAL; retval = oprofilefs_ulong_from_user(&value, buf, count); - if (retval) + if (retval <= 0) return retval; retval = oprofile_set_ulong(file->private_data, value); -- 1.7.7 -- Advanced Micro Devices, Inc. Operating System Research Center |
From: Ingo M. <mi...@el...> - 2011-12-19 16:21:40
|
* Robert Richter <rob...@am...> wrote: > On 19.12.11 15:17:03, Robert Richter wrote: > > On 19.12.11 15:00:49, Ingo Molnar wrote: > > > > > + if (!count) > > > + return 0; > > > + > > > retval = oprofilefs_ulong_from_user(&value, buf, count); > > > if (retval) > > > return retval; > > > > > > See the ugly and fragile pattern? > > > > > > This should *really* be solved via the > > > oprofilefs_ulong_from_user() helper function, not by sprinkling > > > the !count checks in half a dozen places ... > > > > Hmm, I thought there was no way to leave the code path with count == 0 > > and retval. But thinking about it it would be possible with returning > > count or errors < 0. Will improve the patch. > > Ingo, see my updated version below. [...] Yeah, this looks a lot cleaner. > [...] I am fine with appling it directly on tip/perf/urgent. Applied, Thanks, Ingo |
From: Robert R. <rob...@am...> - 2011-12-22 15:15:58
|
On 19.12.11 14:46:12, Robert Richter wrote: > please pull this one fix for 3.2. Ingo, yet another fix. Please apply to tip/perf/urgent. Thanks, -Robert >From a5ea1e51bb66c6081d081cd88d787f248e07d6af Mon Sep 17 00:00:00 2001 From: Vladimir Zapolskiy <vla...@no...> Date: Thu, 22 Dec 2011 15:36:56 +0200 Subject: [PATCH] oprofile, arm/sh: Fix oprofile_arch_exit() linkage issue This change fixes a linking problem, which happens if oprofile is selected to be compiled as built-in: `oprofile_arch_exit' referenced in section `.init.text' of arch/arm/oprofile/built-in.o: defined in discarded section `.exit.text' of arch/arm/oprofile/built-in.o The problem is appeared after commit 87121ca504, which introduced oprofile_arch_exit() calls from __init function. Note that the aforementioned commit has been backported to stable branches, and the problem is known to be reproduced at least with 3.0.13 and 3.1.5 kernels. Cc: st...@ke... # 3.0+ Cc: Will Deacon <wil...@ar...> Signed-off-by: Vladimir Zapolskiy <vla...@no...> Signed-off-by: Robert Richter <rob...@am...> --- arch/arm/oprofile/common.c | 2 +- arch/sh/oprofile/common.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c index c074e66..4e0a371 100644 --- a/arch/arm/oprofile/common.c +++ b/arch/arm/oprofile/common.c @@ -116,7 +116,7 @@ int __init oprofile_arch_init(struct oprofile_operations *ops) return oprofile_perf_init(ops); } -void __exit oprofile_arch_exit(void) +void oprofile_arch_exit(void) { oprofile_perf_exit(); } diff --git a/arch/sh/oprofile/common.c b/arch/sh/oprofile/common.c index b4c2d2b..e4dd5d5 100644 --- a/arch/sh/oprofile/common.c +++ b/arch/sh/oprofile/common.c @@ -49,7 +49,7 @@ int __init oprofile_arch_init(struct oprofile_operations *ops) return oprofile_perf_init(ops); } -void __exit oprofile_arch_exit(void) +void oprofile_arch_exit(void) { oprofile_perf_exit(); kfree(sh_pmu_op_name); @@ -60,5 +60,5 @@ int __init oprofile_arch_init(struct oprofile_operations *ops) ops->backtrace = sh_backtrace; return -ENODEV; } -void __exit oprofile_arch_exit(void) {} +void oprofile_arch_exit(void) {} #endif /* CONFIG_HW_PERF_EVENTS */ -- 1.7.8 -- Advanced Micro Devices, Inc. Operating System Research Center |
From: tip-bot f. V. Z. <vla...@no...> - 2011-12-23 13:19:50
|
Commit-ID: 55205c916e179e09773d98d290334d319f45ac6b Gitweb: http://git.kernel.org/tip/55205c916e179e09773d98d290334d319f45ac6b Author: Vladimir Zapolskiy <vla...@no...> AuthorDate: Thu, 22 Dec 2011 16:15:40 +0100 Committer: Ingo Molnar <mi...@el...> CommitDate: Fri, 23 Dec 2011 11:58:34 +0100 oprofile, arm/sh: Fix oprofile_arch_exit() linkage issue This change fixes a linking problem, which happens if oprofile is selected to be compiled as built-in: `oprofile_arch_exit' referenced in section `.init.text' of arch/arm/oprofile/built-in.o: defined in discarded section `.exit.text' of arch/arm/oprofile/built-in.o The problem is appeared after commit 87121ca504, which introduced oprofile_arch_exit() calls from __init function. Note that the aforementioned commit has been backported to stable branches, and the problem is known to be reproduced at least with 3.0.13 and 3.1.5 kernels. Signed-off-by: Vladimir Zapolskiy <vla...@no...> Signed-off-by: Robert Richter <rob...@am...> Cc: Will Deacon <wil...@ar...> Cc: oprofile-list <opr...@li...> Cc: <st...@ke...> Link: http://lkml.kernel.org/r/201...@er... Signed-off-by: Ingo Molnar <mi...@el...> --- arch/arm/oprofile/common.c | 2 +- arch/sh/oprofile/common.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c index c074e66..4e0a371 100644 --- a/arch/arm/oprofile/common.c +++ b/arch/arm/oprofile/common.c @@ -116,7 +116,7 @@ int __init oprofile_arch_init(struct oprofile_operations *ops) return oprofile_perf_init(ops); } -void __exit oprofile_arch_exit(void) +void oprofile_arch_exit(void) { oprofile_perf_exit(); } diff --git a/arch/sh/oprofile/common.c b/arch/sh/oprofile/common.c index b4c2d2b..e4dd5d5 100644 --- a/arch/sh/oprofile/common.c +++ b/arch/sh/oprofile/common.c @@ -49,7 +49,7 @@ int __init oprofile_arch_init(struct oprofile_operations *ops) return oprofile_perf_init(ops); } -void __exit oprofile_arch_exit(void) +void oprofile_arch_exit(void) { oprofile_perf_exit(); kfree(sh_pmu_op_name); @@ -60,5 +60,5 @@ int __init oprofile_arch_init(struct oprofile_operations *ops) ops->backtrace = sh_backtrace; return -ENODEV; } -void __exit oprofile_arch_exit(void) {} +void oprofile_arch_exit(void) {} #endif /* CONFIG_HW_PERF_EVENTS */ |