Re: [LTP] [PATCH] fix sys_prctl() returned uninitialized value
-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-
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list
Re: [LTP] [PATCH] fix sys_prctl() returned uninitialized value
Quoting Andrew Morton ([EMAIL PROTECTED]):
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).
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 [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]
Acked-by: Serge 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;
_
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list
[LTP] [PATCH] 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]
---
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;
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list
Re: [LTP] [PATCH] fix sys_prctl() returned uninitialized value
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]
---
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?
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list
Re: [LTP] [PATCH] fix sys_prctl() returned uninitialized value
Andrew Morton wrote:
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]
---
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)
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list
Re: [LTP] [PATCH] fix sys_prctl() returned uninitialized value
On Thu, 22 May 2008 12:34:59 +0800 Li Zefan [EMAIL PROTECTED] wrote:
Andrew Morton wrote:
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]
---
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.
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list
Re: [LTP] [PATCH] fix sys_prctl() returned uninitialized value
-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(-)
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
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/___
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list
Re: [LTP] [PATCH] fix sys_prctl() returned uninitialized value
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;
_
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list
