[PATCH][next] clone3: fix an unsigned args.cgroup comparison to less than zero

[PATCH][next] clone3: fix an unsigned args.cgroup comparison to less than zero

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

The less than zero comparison of args.cgroup is aways false because
args.cgroup is a u64 and can never be less than zero.  I believe the
correct check is to cast args.cgroup to a s64 first to ensure an
invalid value is not copied to kargs->cgroup.

Addresses-Coverity: ("Unsigned compared against 0")
Fixes: ef2c41cf38a7 ("clone3: allow spawning processes into cgroups")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 kernel/fork.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 67a5d691ffa8..98513a122dd1 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2635,7 +2635,7 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
 		     !valid_signal(args.exit_signal)))
 		return -EINVAL;
 
-	if ((args.flags & CLONE_INTO_CGROUP) && args.cgroup < 0)
+	if ((args.flags & CLONE_INTO_CGROUP) && (s64)args.cgroup < 0)
 		return -EINVAL;
 
 	*kargs = (struct kernel_clone_args){
-- 
2.25.0

Re: [PATCH][next] clone3: fix an unsigned args.cgroup comparison to less than zero

[Christian Brauner]

On Sat, Feb 22, 2020 at 12:15:13AM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> The less than zero comparison of args.cgroup is aways false because
> args.cgroup is a u64 and can never be less than zero.  I believe the
> correct check is to cast args.cgroup to a s64 first to ensure an
> invalid value is not copied to kargs->cgroup.
> 
> Addresses-Coverity: ("Unsigned compared against 0")
> Fixes: ef2c41cf38a7 ("clone3: allow spawning processes into cgroups")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>

Thanks, Colin.
Dan has reported this issue a few days prior on the janitors list so he
likely should get a 
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
too. I've been sending Dan's bug report to the kernel-mentee list too in
case someone wanted to fix it there. So I'm Ccing them again here so
they know someone's working on this!

> ---
>  kernel/fork.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 67a5d691ffa8..98513a122dd1 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2635,7 +2635,7 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
>  		     !valid_signal(args.exit_signal)))
>  		return -EINVAL;
>  
> -	if ((args.flags & CLONE_INTO_CGROUP) && args.cgroup < 0)
> +	if ((args.flags & CLONE_INTO_CGROUP) && (s64)args.cgroup < 0)

I think my code here needs a little more fixing. I'm doing

	if ((args.flags & CLONE_INTO_CGROUP) && args.cgroup < 0)
		return -EINVAL;

and then later

	*kargs = (struct kernel_clone_args){
		.cgroup		= args.cgroup,
	};

blindly casting from a u64 in struct clone_args to an int in struct
kernel_clone_args. I think we want to check that we're within a sane
range. Maybe something like:

diff --git a/kernel/fork.c b/kernel/fork.c
index 2diff --git a/kernel/fork.c b/kernel/fork.c
index 2853e258fe1f..dca4dde3b5b2 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2618,7 +2618,8 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
                     !valid_signal(args.exit_signal)))
                return -EINVAL;

-       if ((args.flags & CLONE_INTO_CGROUP) && args.cgroup < 0)
+       if ((args.flags & CLONE_INTO_CGROUP) &&
+           (args.cgroup > INT_MAX || (s64)args.cgroup < 0))
                return -EINVAL;

        *kargs = (struct kernel_clone_args){

Christian

Re: [PATCH][next] clone3: fix an unsigned args.cgroup comparison to less than zero

[Dan Carpenter]

On Sat, Feb 22, 2020 at 01:18:01PM +0100, Christian Brauner wrote:
> On Sat, Feb 22, 2020 at 12:15:13AM +0000, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > The less than zero comparison of args.cgroup is aways false because
> > args.cgroup is a u64 and can never be less than zero.  I believe the
> > correct check is to cast args.cgroup to a s64 first to ensure an
> > invalid value is not copied to kargs->cgroup.
> > 
> > Addresses-Coverity: ("Unsigned compared against 0")
> > Fixes: ef2c41cf38a7 ("clone3: allow spawning processes into cgroups")
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> 
> Thanks, Colin.
> Dan has reported this issue a few days prior on the janitors list so he
> likely should get a 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> too.

Colin found it independently so no need for a Reported-by.

> diff --git a/kernel/fork.c b/kernel/fork.c
> index 2diff --git a/kernel/fork.c b/kernel/fork.c
> index 2853e258fe1f..dca4dde3b5b2 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2618,7 +2618,8 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
>                      !valid_signal(args.exit_signal)))
>                 return -EINVAL;
> 
> -       if ((args.flags & CLONE_INTO_CGROUP) && args.cgroup < 0)
> +       if ((args.flags & CLONE_INTO_CGROUP) &&
> +           (args.cgroup > INT_MAX || (s64)args.cgroup < 0))

If we're capping it at INT_MAX then the check for negative isn't
required and static analysis tools know it's not so they might complain.

regards,
dan carpenter

Re: [PATCH][next] clone3: fix an unsigned args.cgroup comparison to less than zero

[Christian Brauner]

On Mon, Feb 24, 2020 at 10:31:57AM +0300, Dan Carpenter wrote:
> On Sat, Feb 22, 2020 at 01:18:01PM +0100, Christian Brauner wrote:
> > On Sat, Feb 22, 2020 at 12:15:13AM +0000, Colin King wrote:
> > > From: Colin Ian King <colin.king@canonical.com>
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 2diff --git a/kernel/fork.c b/kernel/fork.c
> > index 2853e258fe1f..dca4dde3b5b2 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -2618,7 +2618,8 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
> >                      !valid_signal(args.exit_signal)))
> >                 return -EINVAL;
> > 
> > -       if ((args.flags & CLONE_INTO_CGROUP) && args.cgroup < 0)
> > +       if ((args.flags & CLONE_INTO_CGROUP) &&
> > +           (args.cgroup > INT_MAX || (s64)args.cgroup < 0))
> 
> If we're capping it at INT_MAX then the check for negative isn't
> required and static analysis tools know it's not so they might complain.

It isn't, but it's easier to understand for the reader. But I don't care
that much and if it's trouble for tools than fine.

Christian

Re: [PATCH][next] clone3: fix an unsigned args.cgroup comparison to less than zero

[Dan Carpenter]

On Mon, Feb 24, 2020 at 01:25:03PM +0100, Christian Brauner wrote:
> On Mon, Feb 24, 2020 at 10:31:57AM +0300, Dan Carpenter wrote:
> > On Sat, Feb 22, 2020 at 01:18:01PM +0100, Christian Brauner wrote:
> > > On Sat, Feb 22, 2020 at 12:15:13AM +0000, Colin King wrote:
> > > > From: Colin Ian King <colin.king@canonical.com>
> > > diff --git a/kernel/fork.c b/kernel/fork.c
> > > index 2diff --git a/kernel/fork.c b/kernel/fork.c
> > > index 2853e258fe1f..dca4dde3b5b2 100644
> > > --- a/kernel/fork.c
> > > +++ b/kernel/fork.c
> > > @@ -2618,7 +2618,8 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
> > >                      !valid_signal(args.exit_signal)))
> > >                 return -EINVAL;
> > > 
> > > -       if ((args.flags & CLONE_INTO_CGROUP) && args.cgroup < 0)
> > > +       if ((args.flags & CLONE_INTO_CGROUP) &&
> > > +           (args.cgroup > INT_MAX || (s64)args.cgroup < 0))
> > 
> > If we're capping it at INT_MAX then the check for negative isn't
> > required and static analysis tools know it's not so they might complain.
> 
> It isn't, but it's easier to understand for the reader. But I don't care
> that much and if it's trouble for tools than fine.

It's not trouble for tools, (the tools parse it correctly), it's trouble
for me looking at the static checker warnings...

regards,
dan carpenter