* [PATCH] fix sys_prctl() returned uninitialized value @ 2008-05-22 3:19 Shi Weihua 2008-05-22 3:32 ` Andrew Morton 0 siblings, 1 reply; 9+ messages in thread From: Shi Weihua @ 2008-05-22 3:19 UTC (permalink / raw) To: Andrew Morton Cc: Serge E. Hallyn, morgan, linux-security-module, LKML, jmorris, ltp-list When we test kernel by the latest LTP(20080430) on ia64, the following failure occured: ------------------------------------- prctl01 1 PASS : Test Passed prctl01 0 WARN : prctl() returned 2048 errno = 0 : Success prctl01 1 PASS : Test Passed prctl01 2 FAIL : Test Failed ------------------------------------- We found commit 3898b1b4ebff8dcfbcf1807e0661585e06c9a91c causes this failure by git-bisect. And, we found 'error' has not been initialized in the function sys_prctl()(kernel/sys.c). When the capability module is not taking responsibility for this call, sys_prctl() may return a wrong value. Now, i fixed it. Signed-off-by: Shi Weihua <shiwh@cn.fujitsu.com> --- diff --git a/kernel/sys.c b/kernel/sys.c index 895d2d4..26eb0f7 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -1652,7 +1652,7 @@ asmlinkage long sys_umask(int mask) asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3, unsigned long arg4, unsigned long arg5) { - long uninitialized_var(error); + long error = 0; if (security_task_prctl(option, arg2, arg3, arg4, arg5, &error)) return error; ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] fix sys_prctl() returned uninitialized value 2008-05-22 3:19 [PATCH] fix sys_prctl() returned uninitialized value Shi Weihua @ 2008-05-22 3:32 ` Andrew Morton 2008-05-22 4:34 ` [LTP] " Li Zefan 2008-05-22 5:01 ` Andrew G. Morgan 0 siblings, 2 replies; 9+ messages in thread From: Andrew Morton @ 2008-05-22 3:32 UTC (permalink / raw) To: Shi Weihua Cc: Serge E. Hallyn, morgan, linux-security-module, LKML, jmorris, ltp-list On Thu, 22 May 2008 11:19:21 +0800 Shi Weihua <shiwh@cn.fujitsu.com> wrote: > When we test kernel by the latest LTP(20080430) on ia64, > the following failure occured: > ------------------------------------- > prctl01 1 PASS : Test Passed > prctl01 0 WARN : prctl() returned 2048 errno = 0 : Success > prctl01 1 PASS : Test Passed > prctl01 2 FAIL : Test Failed > ------------------------------------- > > We found commit 3898b1b4ebff8dcfbcf1807e0661585e06c9a91c > causes this failure by git-bisect. > And, we found 'error' has not been initialized in the function > sys_prctl()(kernel/sys.c). When the capability module is not taking > responsibility for this call, sys_prctl() may return a wrong value. > > Now, i fixed it. > > Signed-off-by: Shi Weihua <shiwh@cn.fujitsu.com> > --- > diff --git a/kernel/sys.c b/kernel/sys.c > index 895d2d4..26eb0f7 100644 > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -1652,7 +1652,7 @@ asmlinkage long sys_umask(int mask) > asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3, > unsigned long arg4, unsigned long arg5) > { > - long uninitialized_var(error); > + long error = 0; > > if (security_task_prctl(option, arg2, arg3, arg4, arg5, &error)) > return error; Oh dear, there are so many things wrong with this... - if security_task_prctl() is returning "fail" then why on earth isn't it setting the error code? - cap_task_prctl() _does_ set `error' is if returns non-zero, so it must be one of the other myriad backend implementations of security_task_prctl() which is busted. Which one is it? - With the above patch applied, sys_prctl() will return zero (ie: "success") even though it just failed. - Can't we remove the sixth argument to security_task_prctl() and just return the result code like a sane function would do? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [LTP] [PATCH] fix sys_prctl() returned uninitialized value 2008-05-22 3:32 ` Andrew Morton @ 2008-05-22 4:34 ` Li Zefan 2008-05-22 4:57 ` Andrew Morton 2008-05-22 5:01 ` Andrew G. Morgan 1 sibling, 1 reply; 9+ messages in thread From: Li Zefan @ 2008-05-22 4:34 UTC (permalink / raw) To: Andrew Morton Cc: Shi Weihua, ltp-list, LKML, jmorris, linux-security-module, morgan Andrew Morton wrote: > On Thu, 22 May 2008 11:19:21 +0800 Shi Weihua <shiwh@cn.fujitsu.com> wrote: > >> When we test kernel by the latest LTP(20080430) on ia64, >> the following failure occured: >> ------------------------------------- >> prctl01 1 PASS : Test Passed >> prctl01 0 WARN : prctl() returned 2048 errno = 0 : Success >> prctl01 1 PASS : Test Passed >> prctl01 2 FAIL : Test Failed >> ------------------------------------- >> >> We found commit 3898b1b4ebff8dcfbcf1807e0661585e06c9a91c >> causes this failure by git-bisect. >> And, we found 'error' has not been initialized in the function >> sys_prctl()(kernel/sys.c). When the capability module is not taking >> responsibility for this call, sys_prctl() may return a wrong value. >> >> Now, i fixed it. >> >> Signed-off-by: Shi Weihua <shiwh@cn.fujitsu.com> >> --- >> diff --git a/kernel/sys.c b/kernel/sys.c >> index 895d2d4..26eb0f7 100644 >> --- a/kernel/sys.c >> +++ b/kernel/sys.c >> @@ -1652,7 +1652,7 @@ asmlinkage long sys_umask(int mask) >> asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3, >> unsigned long arg4, unsigned long arg5) >> { >> - long uninitialized_var(error); >> + long error = 0; >> >> if (security_task_prctl(option, arg2, arg3, arg4, arg5, &error)) >> return error; > > Oh dear, there are so many things wrong with this... > > - if security_task_prctl() is returning "fail" then why on earth > isn't it setting the error code? > See comments in security.h: * @task_prctl: ... * @rc_p contains a pointer to communicate back the forced return code * Return 0 if permission is granted, and non-zero if the security module * has taken responsibility (setting *rc_p) for the prctl call. But I don't know why can't just set *rc_p to 0 before returning 0 (as Shi's previous patch did). > - cap_task_prctl() _does_ set `error' is if returns non-zero, so it > must be one of the other myriad backend implementations of > security_task_prctl() which is busted. Which one is it? > > - With the above patch applied, sys_prctl() will return zero (ie: > "success") even though it just failed. > This won't happen. We initialize error to 0, and it will be set to some error value when it should be. The alternative is to set error to -EFXXX or 0 in every switch cases. > - Can't we remove the sixth argument to security_task_prctl() and > just return the result code like a sane function would do? > It used to have 5 arguments, but this commit changed it (and caused this ltp failure): 3898b1b4ebff8dcfbcf1807e0661585e06c9a91c (capabilities: implement per-process securebits) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [LTP] [PATCH] fix sys_prctl() returned uninitialized value 2008-05-22 4:34 ` [LTP] " Li Zefan @ 2008-05-22 4:57 ` Andrew Morton 0 siblings, 0 replies; 9+ messages in thread From: Andrew Morton @ 2008-05-22 4:57 UTC (permalink / raw) To: Li Zefan; +Cc: Shi Weihua, ltp-list, LKML, jmorris, linux-security-module, morgan On Thu, 22 May 2008 12:34:59 +0800 Li Zefan <lizf@cn.fujitsu.com> wrote: > Andrew Morton wrote: > > On Thu, 22 May 2008 11:19:21 +0800 Shi Weihua <shiwh@cn.fujitsu.com> wrote: > > > >> When we test kernel by the latest LTP(20080430) on ia64, > >> the following failure occured: > >> ------------------------------------- > >> prctl01 1 PASS : Test Passed > >> prctl01 0 WARN : prctl() returned 2048 errno = 0 : Success > >> prctl01 1 PASS : Test Passed > >> prctl01 2 FAIL : Test Failed > >> ------------------------------------- > >> > >> We found commit 3898b1b4ebff8dcfbcf1807e0661585e06c9a91c > >> causes this failure by git-bisect. > >> And, we found 'error' has not been initialized in the function > >> sys_prctl()(kernel/sys.c). When the capability module is not taking > >> responsibility for this call, sys_prctl() may return a wrong value. > >> > >> Now, i fixed it. > >> > >> Signed-off-by: Shi Weihua <shiwh@cn.fujitsu.com> > >> --- > >> diff --git a/kernel/sys.c b/kernel/sys.c > >> index 895d2d4..26eb0f7 100644 > >> --- a/kernel/sys.c > >> +++ b/kernel/sys.c > >> @@ -1652,7 +1652,7 @@ asmlinkage long sys_umask(int mask) > >> asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3, > >> unsigned long arg4, unsigned long arg5) > >> { > >> - long uninitialized_var(error); > >> + long error = 0; > >> > >> if (security_task_prctl(option, arg2, arg3, arg4, arg5, &error)) > >> return error; > > > > Oh dear, there are so many things wrong with this... > > > > - if security_task_prctl() is returning "fail" then why on earth > > isn't it setting the error code? > > > > See comments in security.h: > > * @task_prctl: > ... > * @rc_p contains a pointer to communicate back the forced return code > * Return 0 if permission is granted, and non-zero if the security module > * has taken responsibility (setting *rc_p) for the prctl call. But that didn't happen! As you've discovered, security_task_prctl() returns non-zero but _doesn't_ set *rc_p. Which, as I said, is inconsistent with cap_task_prctl(). As well as being downright peculiar. > But I don't know why can't just set *rc_p to 0 before returning 0 (as Shi's previous > patch did). Zeroing it seems wrong. It should return the _reason_ for the non-zero return? > > - cap_task_prctl() _does_ set `error' is if returns non-zero, so it > > must be one of the other myriad backend implementations of > > security_task_prctl() which is busted. Which one is it? > > > > - With the above patch applied, sys_prctl() will return zero (ie: > > "success") even though it just failed. > > > > This won't happen. We initialize error to 0, and it will be set to some error > value when it should be. If that were true, we'd never be returning an uninitialised value. Unless there's some code somewhere which is doing if (*rc_p != 0) *rc_p = something; which there might be. In which case the entire patch makes complete sense, but a) it needs changelog repair and b) I'd suggest that the zeroing should be done in security_task_prctl() instead and c) someone needs to work out what the actual interface is to this thing and document it properly. If that interface is deemed to be "randomly returns inexplicable garbage in `error'" then fine, perhaps sys_prctl() should just return literal zero in this case and ignore `error'. Even though returning zero in that case seems exactly wrong. > The alternative is to set error to -EFXXX or 0 in every > switch cases. > > > - Can't we remove the sixth argument to security_task_prctl() and > > just return the result code like a sane function would do? > > > > It used to have 5 arguments, but this commit changed it (and caused this ltp failure): > 3898b1b4ebff8dcfbcf1807e0661585e06c9a91c > (capabilities: implement per-process securebits) hm, I suppose there was a reason. Sorry, but the whole thing looks screwed up to me. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fix sys_prctl() returned uninitialized value 2008-05-22 3:32 ` Andrew Morton 2008-05-22 4:34 ` [LTP] " Li Zefan @ 2008-05-22 5:01 ` Andrew G. Morgan 2008-05-22 5:15 ` Andrew Morton 2008-05-22 5:25 ` Andrew Morton 1 sibling, 2 replies; 9+ messages in thread From: Andrew G. Morgan @ 2008-05-22 5:01 UTC (permalink / raw) To: Andrew Morton Cc: Shi Weihua, Serge E. Hallyn, linux-security-module, LKML, jmorris, ltp-list [-- Attachment #1: Type: text/plain, Size: 1564 bytes --] -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Looks like I goofed here. :*( Andrew Morton wrote: | Oh dear, there are so many things wrong with this... | | - if security_task_prctl() is returning "fail" then why on earth | isn't it setting the error code? Its not failing, as Shi points out in their patch preamble, its simply passing-through - security_task_prctl() doesn't implement the requested PR_* code, so it expects something else (sys_prctl() proper) to set this value. | - cap_task_prctl() _does_ set `error' is if returns non-zero, so it | must be one of the other myriad backend implementations of | security_task_prctl() which is busted. Which one is it? None of them. In this case, none of the security modules implement the requested PRCTL. | - With the above patch applied, sys_prctl() will return zero (ie: | "success") even though it just failed. Not sure what you mean here. The switch statement only sets a non-zero value for error on a failing path. It assumes that the error value is initially zero. | - Can't we remove the sixth argument to security_task_prctl() and | just return the result code like a sane function would do? A bunch of capability related prctl()s will cease to work. I'd prefer the attached patch, but I don't object to Shi's. In which case: ~ Acked-by: Andrew G. Morgan <morgan@kernel.org> Cheers Andrew -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.6 (GNU/Linux) iD8DBQFINP4d+bHCR3gb8jsRAj9pAJ4g8WqzSOomhIirAdjt2nZ//mCAoACcDA+0 EKUYQcvgTgbPig1erxmglsA= =n5ae -----END PGP SIGNATURE----- [-- Attachment #2: Bug-fix-default-error-to-success.patch --] [-- Type: text/plain, Size: 794 bytes --] From 5064e50b4a10cef2fe48a5716ffb3845488f0a14 Mon Sep 17 00:00:00 2001 From: Andrew G. Morgan <morgan@kernel.org> Date: Wed, 21 May 2008 21:46:35 -0700 Subject: [PATCH] Bug fix: default error to success this is the default expected by the subsequent switch (). Signed-off-by: Andrew G. Morgan <morgan@kernel.org> --- kernel/sys.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/kernel/sys.c b/kernel/sys.c index 895d2d4..cb25a64 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -1657,6 +1657,8 @@ asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3, if (security_task_prctl(option, arg2, arg3, arg4, arg5, &error)) return error; + error = 0; + switch (option) { case PR_SET_PDEATHSIG: if (!valid_signal(arg2)) { -- 1.5.3.7 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] fix sys_prctl() returned uninitialized value 2008-05-22 5:01 ` Andrew G. Morgan @ 2008-05-22 5:15 ` Andrew Morton 2008-05-22 5:25 ` Andrew Morton 1 sibling, 0 replies; 9+ messages in thread From: Andrew Morton @ 2008-05-22 5:15 UTC (permalink / raw) To: Andrew G. Morgan Cc: Shi Weihua, Serge E. Hallyn, linux-security-module, LKML, jmorris, ltp-list On Wed, 21 May 2008 22:01:17 -0700 "Andrew G. Morgan" <morgan@kernel.org> wrote: > this is the default expected by the subsequent switch (). > oh gawd. > > diff --git a/kernel/sys.c b/kernel/sys.c > index 895d2d4..cb25a64 100644 > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -1657,6 +1657,8 @@ asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3, > if (security_task_prctl(option, arg2, arg3, arg4, arg5, &error)) > return error; All along I was believing that it was _this_ return which was causing the problem. > + error = 0; > + > switch (option) { > case PR_SET_PDEATHSIG: > if (!valid_signal(arg2)) { But now I see it. This was a hard way to write a changelog. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fix sys_prctl() returned uninitialized value 2008-05-22 5:01 ` Andrew G. Morgan 2008-05-22 5:15 ` Andrew Morton @ 2008-05-22 5:25 ` Andrew Morton 2008-05-22 13:07 ` Andrew G. Morgan 2008-05-22 19:17 ` Serge E. Hallyn 1 sibling, 2 replies; 9+ messages in thread From: Andrew Morton @ 2008-05-22 5:25 UTC (permalink / raw) To: Andrew G. Morgan Cc: Shi Weihua, Serge E. Hallyn, linux-security-module, LKML, jmorris, ltp-list On Wed, 21 May 2008 22:01:17 -0700 "Andrew G. Morgan" <morgan@kernel.org> wrote: > this is the default expected by the subsequent switch (). > > Signed-off-by: Andrew G. Morgan <morgan@kernel.org> > --- > kernel/sys.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/kernel/sys.c b/kernel/sys.c > index 895d2d4..cb25a64 100644 > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -1657,6 +1657,8 @@ asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3, > if (security_task_prctl(option, arg2, arg3, arg4, arg5, &error)) > return error; > > + error = 0; > + > switch (option) { > case PR_SET_PDEATHSIG: > if (!valid_signal(arg2)) { Looking at it some more there are two cases which don't initialise `error': PR_SET_PDEATHSIG and PR_SET_DUMPABLE. (let's set aside the silliness of having sys_prctl() perform set_dumpable()'s argument checking for it). So I would propose this fix, mainly because it removes that nasty uninitialized_var(). Please review carefully. From: Shi Weihua <shiwh@cn.fujitsu.com> If none of the switch cases match, the PR_SET_PDEATHSIG and PR_SET_DUMPABLE cases of the switch statement will never write to local variable `error'. Signed-off-by: Shi Weihua <shiwh@cn.fujitsu.com> Cc: Andrew G. Morgan <morgan@kernel.org> Cc: "Serge E. Hallyn" <serue@us.ibm.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- kernel/sys.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff -puN kernel/sys.c~sys_prctl-fix-return-of-uninitialized-value kernel/sys.c --- a/kernel/sys.c~sys_prctl-fix-return-of-uninitialized-value +++ a/kernel/sys.c @@ -1652,7 +1652,7 @@ asmlinkage long sys_umask(int mask) asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3, unsigned long arg4, unsigned long arg5) { - long uninitialized_var(error); + long error = 0; if (security_task_prctl(option, arg2, arg3, arg4, arg5, &error)) return error; @@ -1701,9 +1701,7 @@ asmlinkage long sys_prctl(int option, un error = PR_TIMING_STATISTICAL; break; case PR_SET_TIMING: - if (arg2 == PR_TIMING_STATISTICAL) - error = 0; - else + if (arg2 != PR_TIMING_STATISTICAL) error = -EINVAL; break; _ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fix sys_prctl() returned uninitialized value 2008-05-22 5:25 ` Andrew Morton @ 2008-05-22 13:07 ` Andrew G. Morgan 2008-05-22 19:17 ` Serge E. Hallyn 1 sibling, 0 replies; 9+ messages in thread From: Andrew G. Morgan @ 2008-05-22 13:07 UTC (permalink / raw) To: Andrew Morton Cc: Shi Weihua, Serge E. Hallyn, linux-security-module, LKML, jmorris, ltp-list -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Acked-by: Andrew G. Morgan <morgan@kernel.org> Cheers Andrew Andrew Morton wrote: | On Wed, 21 May 2008 22:01:17 -0700 "Andrew G. Morgan" <morgan@kernel.org> wrote: | |> this is the default expected by the subsequent switch (). |> |> Signed-off-by: Andrew G. Morgan <morgan@kernel.org> |> --- |> kernel/sys.c | 2 ++ |> 1 files changed, 2 insertions(+), 0 deletions(-) |> |> diff --git a/kernel/sys.c b/kernel/sys.c |> index 895d2d4..cb25a64 100644 |> --- a/kernel/sys.c |> +++ b/kernel/sys.c |> @@ -1657,6 +1657,8 @@ asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3, |> if (security_task_prctl(option, arg2, arg3, arg4, arg5, &error)) |> return error; |> |> + error = 0; |> + |> switch (option) { |> case PR_SET_PDEATHSIG: |> if (!valid_signal(arg2)) { | | Looking at it some more there are two cases which don't initialise | `error': PR_SET_PDEATHSIG and PR_SET_DUMPABLE. (let's set aside the | silliness of having sys_prctl() perform set_dumpable()'s argument | checking for it). | | So I would propose this fix, mainly because it removes that nasty | uninitialized_var(). Please review carefully. | | | | From: Shi Weihua <shiwh@cn.fujitsu.com> | | If none of the switch cases match, the PR_SET_PDEATHSIG and | PR_SET_DUMPABLE cases of the switch statement will never write to local | variable `error'. | | Signed-off-by: Shi Weihua <shiwh@cn.fujitsu.com> | Cc: Andrew G. Morgan <morgan@kernel.org> | Cc: "Serge E. Hallyn" <serue@us.ibm.com> | Signed-off-by: Andrew Morton <akpm@linux-foundation.org> | --- | | kernel/sys.c | 6 ++---- | 1 file changed, 2 insertions(+), 4 deletions(-) | | diff -puN kernel/sys.c~sys_prctl-fix-return-of-uninitialized-value kernel/sys.c | --- a/kernel/sys.c~sys_prctl-fix-return-of-uninitialized-value | +++ a/kernel/sys.c | @@ -1652,7 +1652,7 @@ asmlinkage long sys_umask(int mask) | asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3, | unsigned long arg4, unsigned long arg5) | { | - long uninitialized_var(error); | + long error = 0; | | if (security_task_prctl(option, arg2, arg3, arg4, arg5, &error)) | return error; | @@ -1701,9 +1701,7 @@ asmlinkage long sys_prctl(int option, un | error = PR_TIMING_STATISTICAL; | break; | case PR_SET_TIMING: | - if (arg2 == PR_TIMING_STATISTICAL) | - error = 0; | - else | + if (arg2 != PR_TIMING_STATISTICAL) | error = -EINVAL; | break; | | _ | | -- | To unsubscribe from this list: send the line "unsubscribe linux-security-module" in | the body of a message to majordomo@vger.kernel.org | More majordomo info at http://vger.kernel.org/majordomo-info.html -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.6 (GNU/Linux) iD8DBQFINXAX+bHCR3gb8jsRAl4JAJ0djex041bKhq2gvcwUdJpfjSt9+gCeI4g0 uh1GZlx9WxoeU4ztcOV2kHI= =c1Dw -----END PGP SIGNATURE----- ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fix sys_prctl() returned uninitialized value 2008-05-22 5:25 ` Andrew Morton 2008-05-22 13:07 ` Andrew G. Morgan @ 2008-05-22 19:17 ` Serge E. Hallyn 1 sibling, 0 replies; 9+ messages in thread From: Serge E. Hallyn @ 2008-05-22 19:17 UTC (permalink / raw) To: Andrew Morton Cc: Andrew G. Morgan, Shi Weihua, Serge E. Hallyn, linux-security-module, LKML, jmorris, ltp-list Quoting Andrew Morton (akpm@linux-foundation.org): > On Wed, 21 May 2008 22:01:17 -0700 "Andrew G. Morgan" <morgan@kernel.org> wrote: > > > this is the default expected by the subsequent switch (). > > > > Signed-off-by: Andrew G. Morgan <morgan@kernel.org> > > --- > > kernel/sys.c | 2 ++ > > 1 files changed, 2 insertions(+), 0 deletions(-) > > > > diff --git a/kernel/sys.c b/kernel/sys.c > > index 895d2d4..cb25a64 100644 > > --- a/kernel/sys.c > > +++ b/kernel/sys.c > > @@ -1657,6 +1657,8 @@ asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3, > > if (security_task_prctl(option, arg2, arg3, arg4, arg5, &error)) > > return error; > > > > + error = 0; > > + > > switch (option) { > > case PR_SET_PDEATHSIG: > > if (!valid_signal(arg2)) { > > Looking at it some more there are two cases which don't initialise > `error': PR_SET_PDEATHSIG and PR_SET_DUMPABLE. (let's set aside the > silliness of having sys_prctl() perform set_dumpable()'s argument > checking for it). Hmm, I don't know what kernel version I was looking at, or whose glasses I was wearing at the time. Clearly these are the two... > So I would propose this fix, mainly because it removes that nasty > uninitialized_var(). Please review carefully. > > > > From: Shi Weihua <shiwh@cn.fujitsu.com> > > If none of the switch cases match, the PR_SET_PDEATHSIG and > PR_SET_DUMPABLE cases of the switch statement will never write to local > variable `error'. > > Signed-off-by: Shi Weihua <shiwh@cn.fujitsu.com> > Cc: Andrew G. Morgan <morgan@kernel.org> > Cc: "Serge E. Hallyn" <serue@us.ibm.com> Acked-by: Serge Hallyn <serue@us.ibm.com> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > --- > > kernel/sys.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff -puN kernel/sys.c~sys_prctl-fix-return-of-uninitialized-value kernel/sys.c > --- a/kernel/sys.c~sys_prctl-fix-return-of-uninitialized-value > +++ a/kernel/sys.c > @@ -1652,7 +1652,7 @@ asmlinkage long sys_umask(int mask) > asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3, > unsigned long arg4, unsigned long arg5) > { > - long uninitialized_var(error); > + long error = 0; > > if (security_task_prctl(option, arg2, arg3, arg4, arg5, &error)) > return error; > @@ -1701,9 +1701,7 @@ asmlinkage long sys_prctl(int option, un > error = PR_TIMING_STATISTICAL; > break; > case PR_SET_TIMING: > - if (arg2 == PR_TIMING_STATISTICAL) > - error = 0; > - else > + if (arg2 != PR_TIMING_STATISTICAL) > error = -EINVAL; > break; > > _ ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2008-05-22 22:58 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-05-22 3:19 [PATCH] fix sys_prctl() returned uninitialized value Shi Weihua 2008-05-22 3:32 ` Andrew Morton 2008-05-22 4:34 ` [LTP] " Li Zefan 2008-05-22 4:57 ` Andrew Morton 2008-05-22 5:01 ` Andrew G. Morgan 2008-05-22 5:15 ` Andrew Morton 2008-05-22 5:25 ` Andrew Morton 2008-05-22 13:07 ` Andrew G. Morgan 2008-05-22 19:17 ` Serge E. Hallyn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox