fix sys_prctl() returned uninitialized value

PATCH In review Shi Weihua 5 participants 9 messages May 22, 2008

Share & tools

Shi Weihua

author

2008-05-22 03:20 UTC #0 lore raw

PATCH 1/1 fix sys_prctl() returned uninitialized value

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 <[email protected]>

---

kernel/sys.c +1 -1

index 895d2d4..26eb0f7 100644
@@ -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;

Andrew Morton

2008-05-22 03:33 UTC #1 lore raw

Quoted 24 lines · On Thu, 22 May 2008 11:19:21 +0800 Shi Weihua <[email protected]> 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 <[email protected]>
---
kernel/sys.c +0 -0
index 895d2d4..26eb0f7 100644

kernel/sys.c asmlinkage long sys_umask(int mask) · line 1658

Quoted 8 lines

kernel/sys.c +1 -1

@@ -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?

Li Zefan

2008-05-22 04:36 UTC #2 lore raw

Quoted 32 lines · Andrew Morton

Quoted 30 lines · On Thu, 22 May 2008 11:19:21 +0800 Shi Weihua <[email protected]> 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 <[email protected]>
---
kernel/sys.c +1 -1
index 895d2d4..26eb0f7 100644
@@ -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;
 

Quoted 8 lines

kernel/sys.c asmlinkage long sys_umask(int mask) · line 1658
Quoted 2 lines
kernel/sys.c +0 -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).

Quoted 7 lines

- 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.

Quoted 3 lines

- 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)

Andrew Morton

2008-05-22 04:58 UTC #3 lore raw

Quoted 42 lines · On Thu, 22 May 2008 12:34:59 +0800 Li Zefan <[email protected]> wrote:

Quoted 32 lines · Andrew Morton
Quoted 30 lines · On Thu, 22 May 2008 11:19:21 +0800 Shi Weihua <[email protected]> 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 <[email protected]>
---
kernel/sys.c +1 -1
index 895d2d4..26eb0f7 100644
@@ -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;
 
Quoted 8 lines
kernel/sys.c asmlinkage long sys_umask(int mask) · line 1658
Quoted 2 lines
kernel/sys.c +0 -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?

Quoted 7 lines

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.

Quoted 2 lines

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?

Quoted 10 lines

Quoted 7 lines
- 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.

Quoted 10 lines

The alternative is to set error to -EFXXX or 0 in every switch cases.
Quoted 3 lines
- 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.

Andrew G. Morgan

2008-05-22 05:01 UTC #4 lore raw

In reply to Andrew Morton

-----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 <[email protected]>

Cheers

Andrew

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)

iD8DBQFINP4d+bHCR3gb8jsRAj9pAJ4g8WqzSOomhIirAdjt2nZ//mCAoACcDA+0 EKUYQcvgTgbPig1erxmglsA=
=n5ae
-----END PGP SIGNATURE-----

From 5064e50b4a10cef2fe48a5716ffb3845488f0a14 Mon Sep 17 00:00:00 2001
From: Andrew G. Morgan <[email protected]>
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 <[email protected]>

---
 kernel/sys.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

kernel/sys.c +2 -0

index 895d2d4..cb25a64 100644
@@ -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)) {

Signature · 1 line

1.5.3.7

Andrew Morton

2008-05-22 05:17 UTC #5 lore raw

Quoted 2 lines · On Wed, 21 May 2008 22:01:17 -0700 "Andrew G. Morgan" <[email protected]> wrote:

this is the default expected by the subsequent switch ().

oh gawd.

kernel/sys.c asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3, · line 1658

Quoted 8 lines

kernel/sys.c +0 -0

index 895d2d4..cb25a64 100644
@@ -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.

kernel/sys.c asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3, · line 1664

Quoted 5 lines

kernel/sys.c +2 -0

+	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.

Andrew Morton

2008-05-22 05:26 UTC #6 lore raw

In reply to Andrew G. Morgan

Quoted 12 lines · On Wed, 21 May 2008 22:01:17 -0700 "Andrew G. Morgan" <[email protected]> wrote:

this is the default expected by the subsequent switch ().
Signed-off-by Andrew G. Morgan <[email protected]>
---
 kernel/sys.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
kernel/sys.c +0 -0
index 895d2d4..cb25a64 100644

kernel/sys.c asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3, · line 1664

Quoted 8 lines

kernel/sys.c +2 -0

@@ -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 <[email protected]>

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 <[email protected]>

Cc Andrew G. Morgan <[email protected]>

Cc "Serge E. Hallyn" <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 kernel/sys.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

kernel/sys.c renamed from kernel/sys.c~sys_prctl-fix-return-of-uninitialized-value +2 -4

diff -puN kernel/sys.c~sys_prctl-fix-return-of-uninitialized-value 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;

Andrew G. Morgan

Acked-by

2008-05-22 13:08 UTC #7 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Acked-by Andrew G. Morgan <[email protected]>

Cheers

Andrew

Andrew Morton wrote:
| On Wed, 21 May 2008 22:01:17 -0700 "Andrew G. Morgan" <[email protected]> wrote:

|
|> this is the default expected by the subsequent switch ().
|>
|> Signed-off-by: Andrew G. Morgan <[email protected]>
|> ---
|>  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 <[email protected]>
|
| 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 <[email protected]>
| Cc: Andrew G. Morgan <[email protected]>
| Cc: "Serge E. Hallyn" <[email protected]>
| Signed-off-by: Andrew Morton <[email protected]>
| ---
|
|  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 [email protected]
| 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-----

Serge E. Hallyn

Acked-by

2008-05-22 22:58 UTC #8 lore raw

In reply to Andrew Morton

Quoted 17 lines · Andrew Morton

Quoted 15 lines · On Wed, 21 May 2008 22:01:17 -0700 "Andrew G. Morgan" <[email protected]> wrote:
this is the default expected by the subsequent switch ().
Signed-off-by Andrew G. Morgan <[email protected]>
---
 kernel/sys.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
kernel/sys.c +0 -0
index 895d2d4..cb25a64 100644
@@ -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;
 

Quoted 10 lines

kernel/sys.c asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3, · line 1664
Quoted 5 lines
kernel/sys.c +2 -0
+	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...

Quoted 7 lines

So I would propose this fix, mainly because it removes that nasty uninitialized_var(). Please review carefully.
From: Shi Weihua <[email protected]>

Quoted 7 lines

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 <[email protected]>
Cc Andrew G. Morgan <[email protected]>
Cc "Serge E. Hallyn" <[email protected]>

Acked-by Serge Hallyn <[email protected]>

Quoted 30 lines

Signed-off-by Andrew Morton <[email protected]>

---

 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

kernel/sys.c renamed from kernel/sys.c~sys_prctl-fix-return-of-uninitialized-value +2 -4

@@ -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;