All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] kernel: arg2 is unsigned long which is never < 0
@ 2013-02-06  8:44 Chen Gang
  2013-02-06  8:56 ` Cyrill Gorcunov
  0 siblings, 1 reply; 8+ messages in thread
From: Chen Gang @ 2013-02-06  8:44 UTC (permalink / raw)
  To: akpm, keescook, serge.hallyn, gorcunov, ebiederm, linux-kernel


  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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] kernel: arg2 is unsigned long which is never < 0
  2013-02-06  8:44 [PATCH] kernel: arg2 is unsigned long which is never < 0 Chen Gang
@ 2013-02-06  8:56 ` Cyrill Gorcunov
  2013-02-06 10:36   ` Chen Gang
  0 siblings, 1 reply; 8+ messages in thread
From: Cyrill Gorcunov @ 2013-02-06  8:56 UTC (permalink / raw)
  To: Chen Gang; +Cc: akpm, keescook, serge.hallyn, ebiederm, linux-kernel

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] kernel: arg2 is unsigned long which is never < 0
  2013-02-06  8:56 ` Cyrill Gorcunov
@ 2013-02-06 10:36   ` Chen Gang
  2013-02-06 15:24     ` Serge Hallyn
                       ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Chen Gang @ 2013-02-06 10:36 UTC (permalink / raw)
  To: Cyrill Gorcunov
  Cc: Chen Gang, akpm, keescook, serge.hallyn, ebiederm, linux-kernel,
	Alan Cox, marcel

于 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] kernel: arg2 is unsigned long which is never < 0
  2013-02-06 10:36   ` Chen Gang
@ 2013-02-06 15:24     ` Serge Hallyn
  2013-02-07  1:54       ` Chen Gang
  2013-02-06 15:35     ` Cyrill Gorcunov
  2013-02-06 19:41     ` Kees Cook
  2 siblings, 1 reply; 8+ messages in thread
From: Serge Hallyn @ 2013-02-06 15:24 UTC (permalink / raw)
  To: Chen Gang
  Cc: Cyrill Gorcunov, akpm, keescook, ebiederm, linux-kernel,
	Alan Cox, marcel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] kernel: arg2 is unsigned long which is never < 0
  2013-02-06 10:36   ` Chen Gang
  2013-02-06 15:24     ` Serge Hallyn
@ 2013-02-06 15:35     ` Cyrill Gorcunov
  2013-02-06 19:41     ` Kees Cook
  2 siblings, 0 replies; 8+ messages in thread
From: Cyrill Gorcunov @ 2013-02-06 15:35 UTC (permalink / raw)
  To: Chen Gang
  Cc: akpm, keescook, serge.hallyn, ebiederm, linux-kernel, Alan Cox, marcel

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] kernel: arg2 is unsigned long which is never < 0
  2013-02-06 10:36   ` Chen Gang
  2013-02-06 15:24     ` Serge Hallyn
  2013-02-06 15:35     ` Cyrill Gorcunov
@ 2013-02-06 19:41     ` Kees Cook
  2013-02-07  1:38       ` Chen Gang
  2 siblings, 1 reply; 8+ messages in thread
From: Kees Cook @ 2013-02-06 19:41 UTC (permalink / raw)
  To: Chen Gang
  Cc: Cyrill Gorcunov, Andrew Morton, Serge Hallyn, Eric W. Biederman,
	linux-kernel, Alan Cox, Marcel Holtmann

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] kernel: arg2 is unsigned long which is never < 0
  2013-02-06 19:41     ` Kees Cook
@ 2013-02-07  1:38       ` Chen Gang
  0 siblings, 0 replies; 8+ messages in thread
From: Chen Gang @ 2013-02-07  1:38 UTC (permalink / raw)
  To: Kees Cook
  Cc: Cyrill Gorcunov, Andrew Morton, Serge Hallyn, Eric W. Biederman,
	linux-kernel, Alan Cox, Marcel Holtmann

于 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] kernel: arg2 is unsigned long which is never < 0
  2013-02-06 15:24     ` Serge Hallyn
@ 2013-02-07  1:54       ` Chen Gang
  0 siblings, 0 replies; 8+ messages in thread
From: Chen Gang @ 2013-02-07  1:54 UTC (permalink / raw)
  To: Serge Hallyn
  Cc: Chen Gang, Cyrill Gorcunov, akpm, keescook, ebiederm,
	linux-kernel, Alan Cox, marcel

于 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2013-02-07  1:54 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-06  8:44 [PATCH] kernel: arg2 is unsigned long which is never < 0 Chen Gang
2013-02-06  8:56 ` Cyrill Gorcunov
2013-02-06 10:36   ` Chen Gang
2013-02-06 15:24     ` Serge Hallyn
2013-02-07  1:54       ` Chen Gang
2013-02-06 15:35     ` Cyrill Gorcunov
2013-02-06 19:41     ` Kees Cook
2013-02-07  1:38       ` Chen Gang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.