[PATCH] kernel: arg2 is unsigned long which is never < 0

[PATCH] kernel: arg2 is unsigned long which is never < 0

[Chen Gang]

  arg2 will never < 0, for its type is 'unsigned long'

  so delete the waste code.


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/sys.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index 24d1ef5..568b9ca 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2027,7 +2027,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 			error = get_dumpable(me->mm);
 			break;
 		case PR_SET_DUMPABLE:
-			if (arg2 < 0 || arg2 > 1) {
+			if (arg2 > 1) {
 				error = -EINVAL;
 				break;
 			}
-- 
1.7.7.6

Re: [PATCH] kernel: arg2 is unsigned long which is never < 0

[Cyrill Gorcunov]

On Wed, Feb 06, 2013 at 04:44:35PM +0800, Chen Gang wrote:
> 
>   arg2 will never < 0, for its type is 'unsigned long'
> 
>   so delete the waste code.
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/sys.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 24d1ef5..568b9ca 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2027,7 +2027,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>  			error = get_dumpable(me->mm);
>  			break;
>  		case PR_SET_DUMPABLE:
> -			if (arg2 < 0 || arg2 > 1) {
> +			if (arg2 > 1) {
>  				error = -EINVAL;
>  				break;
>  			}

I guess

	if (arg2 != SUID_DUMPABLE_DISABLED &&
	    arg2 != SUID_DUMPABLE_ENABLED) {
		error = -EINVAL;
		break;
	}

would be better. Still, current patch looks good to me.

Re: [PATCH] kernel: arg2 is unsigned long which is never < 0

[Chen Gang]

于 2013年02月06日 16:56, Cyrill Gorcunov 写道:
> On Wed, Feb 06, 2013 at 04:44:35PM +0800, Chen Gang wrote:
>> > 
>> > diff --git a/kernel/sys.c b/kernel/sys.c
>> > index 24d1ef5..568b9ca 100644
>> > --- a/kernel/sys.c
>> > +++ b/kernel/sys.c
>> > @@ -2027,7 +2027,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>> >  			error = get_dumpable(me->mm);
>> >  			break;
>> >  		case PR_SET_DUMPABLE:
>> > -			if (arg2 < 0 || arg2 > 1) {
>> > +			if (arg2 > 1) {
>> >  				error = -EINVAL;
>> >  				break;
>> >  			}
> I guess
> 
> 	if (arg2 != SUID_DUMPABLE_DISABLED &&
> 	    arg2 != SUID_DUMPABLE_ENABLED) {
> 		error = -EINVAL;
> 		break;
> 	}
> 
> would be better. Still, current patch looks good to me.

thank you for your suggestion, firstly.

and after read more, it seems a little more complex:
for me, I think it would be better:

 	if (arg2 != SUID_DUMP_DISABLE && arg2 != SUID_DUMP_USER) {
 		error = -EINVAL;
 		break;
 	}


the reason is below:

it has 2 branches:

  branch 1:

    #define SUID_DUMP_DISABLE      0       /* No setuid dumping */
    #define SUID_DUMP_USER         1       /* Dump as user of process */
    #define SUID_DUMP_ROOT         2       /* Dump as root */

    in patch d6e711448137ca3301512cec41a2c2ce852b3d0a
      Signed-of-by Alan Cox in 2005.
      define these constant for using.
      change 2 choices to 3 choices (add a new choice).

    in patch abf75a5033d4da7b8a7e92321d74021d1fcfb502
      Signed-of-by Marcel Holtmann in 2006.
      find and fix a security issue for it.


  branch 2:

    #define SUID_DUMPABLE_DISABLED  0
    #define SUID_DUMPABLE_ENABLED   1
    #define SUID_DUMPABLE_SAFE      2

    in patch 54b501992dd2a839e94e76aa392c392b55080ce8
      Signed-of-by Kees Cook in Jul 30 2012
      define the constants for using
      print warning when detect unsafe core_pattern settings

    in patch 0f4cfb2e4e7a7e4e97a3e90e2ba1062f07fb2cb1
      Signed-of-by Oleg Nesterov in Oct 4 2012
      use SUID_DUMPABLE_ENABLED rather than hardcoded 1

analysing:
  branch 1 and branch 2 have the same values with different macro names.
  branch 1 is much older than branch 2.
  for features:
    branch 1 is for functional feature and bug fix,
    branch 2 is for printing warning and beautifying code.

  it seems:
    branch 2 did not notice the branch 1, before it performs.
    if it noticed, it is meanless to define the new macros.

result:
  still use the macros of branch 1
  and use branch 1 macros instead of branch 2 macros (need an additional patch).

  :-)

  Regards.

-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: arg2 is unsigned long which is never < 0

[Serge Hallyn]

Quoting Chen Gang (gang.chen@asianux.com):
> 于 2013年02月06日 16:56, Cyrill Gorcunov 写道:
> > On Wed, Feb 06, 2013 at 04:44:35PM +0800, Chen Gang wrote:
> >> > 
> >> > diff --git a/kernel/sys.c b/kernel/sys.c
> >> > index 24d1ef5..568b9ca 100644
> >> > --- a/kernel/sys.c
> >> > +++ b/kernel/sys.c
> >> > @@ -2027,7 +2027,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
> >> >  			error = get_dumpable(me->mm);
> >> >  			break;
> >> >  		case PR_SET_DUMPABLE:
> >> > -			if (arg2 < 0 || arg2 > 1) {
> >> > +			if (arg2 > 1) {
> >> >  				error = -EINVAL;
> >> >  				break;
> >> >  			}
> > I guess
> > 
> > 	if (arg2 != SUID_DUMPABLE_DISABLED &&
> > 	    arg2 != SUID_DUMPABLE_ENABLED) {
> > 		error = -EINVAL;
> > 		break;
> > 	}
> > 
> > would be better. Still, current patch looks good to me.
> 
> thank you for your suggestion, firstly.
> 
> and after read more, it seems a little more complex:
> for me, I think it would be better:
> 
>  	if (arg2 != SUID_DUMP_DISABLE && arg2 != SUID_DUMP_USER) {
>  		error = -EINVAL;
>  		break;
>  	}
> 
> 
> the reason is below:
> 
> it has 2 branches:
> 
>   branch 1:
> 
>     #define SUID_DUMP_DISABLE      0       /* No setuid dumping */
>     #define SUID_DUMP_USER         1       /* Dump as user of process */
>     #define SUID_DUMP_ROOT         2       /* Dump as root */
> 
>     in patch d6e711448137ca3301512cec41a2c2ce852b3d0a
>       Signed-of-by Alan Cox in 2005.
>       define these constant for using.
>       change 2 choices to 3 choices (add a new choice).
> 
>     in patch abf75a5033d4da7b8a7e92321d74021d1fcfb502
>       Signed-of-by Marcel Holtmann in 2006.
>       find and fix a security issue for it.
> 
> 
>   branch 2:
> 
>     #define SUID_DUMPABLE_DISABLED  0
>     #define SUID_DUMPABLE_ENABLED   1
>     #define SUID_DUMPABLE_SAFE      2
> 
>     in patch 54b501992dd2a839e94e76aa392c392b55080ce8
>       Signed-of-by Kees Cook in Jul 30 2012
>       define the constants for using
>       print warning when detect unsafe core_pattern settings
> 
>     in patch 0f4cfb2e4e7a7e4e97a3e90e2ba1062f07fb2cb1
>       Signed-of-by Oleg Nesterov in Oct 4 2012
>       use SUID_DUMPABLE_ENABLED rather than hardcoded 1
> 
> analysing:
>   branch 1 and branch 2 have the same values with different macro names.
>   branch 1 is much older than branch 2.
>   for features:
>     branch 1 is for functional feature and bug fix,
>     branch 2 is for printing warning and beautifying code.
> 
>   it seems:
>     branch 2 did not notice the branch 1, before it performs.
>     if it noticed, it is meanless to define the new macros.
> 
> result:
>   still use the macros of branch 1
>   and use branch 1 macros instead of branch 2 macros (need an additional patch).
> 
>   :-)
> 
>   Regards.

This really seems like splitting hairs to me.

Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com>

on the original patch.

thanks,
-serge

Re: [PATCH] kernel: arg2 is unsigned long which is never < 0

[Chen Gang]

于 2013年02月06日 23:24, Serge Hallyn 写道:
> This really seems like splitting hairs to me.
> 
> Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com>
> 
> on the original patch.
> 
> thanks,
> -serge

  thank you.

-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: arg2 is unsigned long which is never < 0

[Cyrill Gorcunov]

On Wed, Feb 06, 2013 at 06:36:38PM +0800, Chen Gang wrote:
...
> 
> result:
>   still use the macros of branch 1
>   and use branch 1 macros instead of branch 2 macros (need an additional patch).

Oh, what a mess ;) The reason I used SUID_DUMPABLE_DISABLED -- because
it's used in set_dumpable which is called directly from this prctl.
I'm fine with either way, plain [0;1] is acceptable as well still
I don;t like it.

Re: [PATCH] kernel: arg2 is unsigned long which is never < 0

[Kees Cook]

On Wed, Feb 6, 2013 at 2:36 AM, Chen Gang <gang.chen@asianux.com> wrote:
> 于 2013年02月06日 16:56, Cyrill Gorcunov 写道:
>> On Wed, Feb 06, 2013 at 04:44:35PM +0800, Chen Gang wrote:
>>> >
>>> > diff --git a/kernel/sys.c b/kernel/sys.c
>>> > index 24d1ef5..568b9ca 100644
>>> > --- a/kernel/sys.c
>>> > +++ b/kernel/sys.c
>>> > @@ -2027,7 +2027,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>>> >                    error = get_dumpable(me->mm);
>>> >                    break;
>>> >            case PR_SET_DUMPABLE:
>>> > -                  if (arg2 < 0 || arg2 > 1) {
>>> > +                  if (arg2 > 1) {
>>> >                            error = -EINVAL;
>>> >                            break;
>>> >                    }
>> I guess
>>
>>       if (arg2 != SUID_DUMPABLE_DISABLED &&
>>           arg2 != SUID_DUMPABLE_ENABLED) {
>>               error = -EINVAL;
>>               break;
>>       }
>>
>> would be better. Still, current patch looks good to me.
>
> thank you for your suggestion, firstly.
>
> and after read more, it seems a little more complex:
> for me, I think it would be better:
>
>         if (arg2 != SUID_DUMP_DISABLE && arg2 != SUID_DUMP_USER) {
>                 error = -EINVAL;
>                 break;
>         }
>
>
> the reason is below:
>
> it has 2 branches:
>
>   branch 1:
>
>     #define SUID_DUMP_DISABLE      0       /* No setuid dumping */
>     #define SUID_DUMP_USER         1       /* Dump as user of process */
>     #define SUID_DUMP_ROOT         2       /* Dump as root */
>
>     in patch d6e711448137ca3301512cec41a2c2ce852b3d0a
>       Signed-of-by Alan Cox in 2005.
>       define these constant for using.
>       change 2 choices to 3 choices (add a new choice).

Ah, good find. These weren't used anywhere in the kernel, so I didn't see them.

>     in patch abf75a5033d4da7b8a7e92321d74021d1fcfb502
>       Signed-of-by Marcel Holtmann in 2006.
>       find and fix a security issue for it.
>
>
>   branch 2:
>
>     #define SUID_DUMPABLE_DISABLED  0
>     #define SUID_DUMPABLE_ENABLED   1
>     #define SUID_DUMPABLE_SAFE      2
>
>     in patch 54b501992dd2a839e94e76aa392c392b55080ce8
>       Signed-of-by Kees Cook in Jul 30 2012
>       define the constants for using
>       print warning when detect unsafe core_pattern settings
>
>     in patch 0f4cfb2e4e7a7e4e97a3e90e2ba1062f07fb2cb1
>       Signed-of-by Oleg Nesterov in Oct 4 2012
>       use SUID_DUMPABLE_ENABLED rather than hardcoded 1
>
> analysing:
>   branch 1 and branch 2 have the same values with different macro names.
>   branch 1 is much older than branch 2.
>   for features:
>     branch 1 is for functional feature and bug fix,
>     branch 2 is for printing warning and beautifying code.
>
>   it seems:
>     branch 2 did not notice the branch 1, before it performs.
>     if it noticed, it is meanless to define the new macros.

Well, it seems we'd need to add an include to gain access to binfmts.h
in once place, but that doesn't seem bad. I'll send to patch to clean
this up.

>
> result:
>   still use the macros of branch 1
>   and use branch 1 macros instead of branch 2 macros (need an additional patch).
>
>   :-)
>
>   Regards.
>
> --
> Chen Gang
>
> Asianux Corporation

-Kees

--
Kees Cook
Chrome OS Security

Re: [PATCH] kernel: arg2 is unsigned long which is never < 0

[Chen Gang]

于 2013年02月07日 03:41, Kees Cook 写道:
> Well, it seems we'd need to add an include to gain access to binfmts.h
> in once place, but that doesn't seem bad. I'll send to patch to clean
> this up.

  thank you.

  and after your patch is integrated into main branch (at least in
next-* branch), I should send patch v2 for it, too.

  and excuse me I will 'disappear' during next 7-10 days for Chinese
Year (Spring Festival).

  during these days, welcome any members to send patch v2 instead of me
(better to mark Reported-by Cyrill Gorcunov <gorcunov@openvz.org>, too)

  thanks.

  :-)

-- 
Chen Gang

Asianux Corporation