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

[Serge Hallyn <serge.hallyn@canonical.com>]

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