The Linux Kernel Mailing List
 help / color / mirror / Atom feed
* [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