[patch] groups: integer underflow in groups_alloc()

[patch] groups: integer underflow in groups_alloc()

[Dan Carpenter]

This is called from rsc_parse() with a use controlled value.  Say for
example that "gidsetsize" is negative, then we could end up allocating
less than sizeof(struct group_info) leading to memory corruption.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I copied the NGROUPS_MAX limit from the surrounding code, I'm not
absolutely that it's the correct limit to use.

diff --git a/kernel/groups.c b/kernel/groups.c
index 664411f..e9341b3 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -18,6 +18,9 @@ struct group_info *groups_alloc(int gidsetsize)
 	int nblocks;
 	int i;
 
+	if ((unsigned)gidsetsize > NGROUPS_MAX)
+		return NULL;
+
 	nblocks = (gidsetsize + NGROUPS_PER_BLOCK - 1) / NGROUPS_PER_BLOCK;
 	/* Make sure we always allocate at least one indirect block pointer */
 	nblocks = nblocks ? : 1;

Re: [patch] groups: integer underflow in groups_alloc()

[Eric W. Biederman]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> This is called from rsc_parse() with a use controlled value.  Say for
> example that "gidsetsize" is negative, then we could end up allocating
> less than sizeof(struct group_info) leading to memory corruption.

Right now it is the responsibility of the caller of groups_alloc to make
certain that gidsetsize is a valid value, and the callers of
groups_alloc who know what they are doing already validate this value.

Either the pattern of caller validates the messages needs to continue,
or groups_alloc needs to be changed and all of the callers need to be
updated.

Changing groups_alloc for one particular caller is just going to cause
maintenance problems.

Eric


> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I copied the NGROUPS_MAX limit from the surrounding code, I'm not
> absolutely that it's the correct limit to use.
>
> diff --git a/kernel/groups.c b/kernel/groups.c
> index 664411f..e9341b3 100644
> --- a/kernel/groups.c
> +++ b/kernel/groups.c
> @@ -18,6 +18,9 @@ struct group_info *groups_alloc(int gidsetsize)
>  	int nblocks;
>  	int i;
>  
> +	if ((unsigned)gidsetsize > NGROUPS_MAX)
> +		return NULL;
> +
>  	nblocks = (gidsetsize + NGROUPS_PER_BLOCK - 1) / NGROUPS_PER_BLOCK;
>  	/* Make sure we always allocate at least one indirect block pointer */
>  	nblocks = nblocks ? : 1;

Re: [patch] groups: integer underflow in groups_alloc()

[Dan Carpenter]

On Mon, Feb 23, 2015 at 11:10:02AM -0600, Eric W. Biederman wrote:
> Dan Carpenter <dan.carpenter@oracle.com> writes:
> 
> > This is called from rsc_parse() with a use controlled value.  Say for
> > example that "gidsetsize" is negative, then we could end up allocating
> > less than sizeof(struct group_info) leading to memory corruption.
> 
> Right now it is the responsibility of the caller of groups_alloc to make
> certain that gidsetsize is a valid value, and the callers of
> groups_alloc who know what they are doing already validate this value.
> 
> Either the pattern of caller validates the messages needs to continue,
> or groups_alloc needs to be changed and all of the callers need to be
> updated.
> 
> Changing groups_alloc for one particular caller is just going to cause
> maintenance problems.
> 

This only affects NFS so let's hear from them if this limit is correct
and decide from there.

regards,
dan carpenter

Re: [patch] groups: integer underflow in groups_alloc()

[Eric W. Biederman]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> On Mon, Feb 23, 2015 at 11:10:02AM -0600, Eric W. Biederman wrote:
>> Dan Carpenter <dan.carpenter@oracle.com> writes:
>> 
>> > This is called from rsc_parse() with a use controlled value.  Say for
>> > example that "gidsetsize" is negative, then we could end up allocating
>> > less than sizeof(struct group_info) leading to memory corruption.
>> 
>> Right now it is the responsibility of the caller of groups_alloc to make
>> certain that gidsetsize is a valid value, and the callers of
>> groups_alloc who know what they are doing already validate this value.
>> 
>> Either the pattern of caller validates the messages needs to continue,
>> or groups_alloc needs to be changed and all of the callers need to be
>> updated.
>> 
>> Changing groups_alloc for one particular caller is just going to cause
>> maintenance problems.
>> 
>
> This only affects NFS so let's hear from them if this limit is correct
> and decide from there.

The bug may be nfs specific bug changing groups_alloc does not only
affect nfs.

NGROUPS_MAX is the maxmimum number of groups the linux kernel supports
so NGROUPS_MAX may be high but it is certainly not wrong.

Your patch takes the wrong approach, creates code that is an
inconsistent mess and is thus wrong.  As setgroups is code that is
called every day I don't think only paying attention to NFS when talking 
how to change this is in any way appropriate, unless you propose an NFS
specific fix (which you clearly did not).

Eric

Re: [patch] groups: integer underflow in groups_alloc()

[J. Bruce Fields]

On Mon, Feb 23, 2015 at 09:03:27PM +0300, Dan Carpenter wrote:
> On Mon, Feb 23, 2015 at 11:10:02AM -0600, Eric W. Biederman wrote:
> > Dan Carpenter <dan.carpenter@oracle.com> writes:
> > 
> > > This is called from rsc_parse() with a use controlled value.  Say for
> > > example that "gidsetsize" is negative, then we could end up allocating
> > > less than sizeof(struct group_info) leading to memory corruption.
> > 
> > Right now it is the responsibility of the caller of groups_alloc to make
> > certain that gidsetsize is a valid value, and the callers of
> > groups_alloc who know what they are doing already validate this value.
> > 
> > Either the pattern of caller validates the messages needs to continue,
> > or groups_alloc needs to be changed and all of the callers need to be
> > updated.
> > 
> > Changing groups_alloc for one particular caller is just going to cause
> > maintenance problems.
> > 
> 
> This only affects NFS so let's hear from them if this limit is correct
> and decide from there.

I think that's probably the correct check, sure.

Putting it in rsc_parse sounds reasonable.

--b.

[patch v2] sunrpc: integer underflow in rsc_parse()

[Dan Carpenter]

If we call groups_alloc() with invalid values then it's might lead to
memory corruption.  For example, with a negative value then we might not
allocate enough for sizeof(struct group_info).

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2:  In v1, I changed groups_alloc().  The other places which call
groups_alloc() check the value before calling.  Eric wanted that, either
have all the callers check, or all the callers rely on groups_alloc().
In the end, Bruce Fields said adding the check here was probably
reasonable.

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 224a82f..1095be9 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -463,6 +463,8 @@ static int rsc_parse(struct cache_detail *cd,
 		/* number of additional gid's */
 		if (get_int(&mesg, &N))
 			goto out;
+		if (N < 0 || N > NGROUPS_MAX)
+			goto out;
 		status = -ENOMEM;
 		rsci.cred.cr_group_info = groups_alloc(N);
 		if (rsci.cred.cr_group_info == NULL)

Re: [patch v2] sunrpc: integer underflow in rsc_parse()

[Simo Sorce]

On Tue, 2015-02-24 at 18:34 +0300, Dan Carpenter wrote:
> If we call groups_alloc() with invalid values then it's might lead to
> memory corruption.  For example, with a negative value then we might not
> allocate enough for sizeof(struct group_info).
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2:  In v1, I changed groups_alloc().  The other places which call
> groups_alloc() check the value before calling.  Eric wanted that, either
> have all the callers check, or all the callers rely on groups_alloc().
> In the end, Bruce Fields said adding the check here was probably
> reasonable.
> 
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index 224a82f..1095be9 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -463,6 +463,8 @@ static int rsc_parse(struct cache_detail *cd,
>  		/* number of additional gid's */
>  		if (get_int(&mesg, &N))
>  			goto out;
> +		if (N < 0 || N > NGROUPS_MAX)
> +			goto out;
>  		status = -ENOMEM;
>  		rsci.cred.cr_group_info = groups_alloc(N);
>  		if (rsci.cred.cr_group_info == NULL)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

I touched this code relatively recently, and this check looks correct.
Feel free to add Reviewed-by: Simo Sorce <simo@redhat.com>

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

Re: [patch v2] sunrpc: integer underflow in rsc_parse()

[J. Bruce Fields]

On Tue, Feb 24, 2015 at 10:54:44PM -0500, Simo Sorce wrote:
> On Tue, 2015-02-24 at 18:34 +0300, Dan Carpenter wrote:
> > If we call groups_alloc() with invalid values then it's might lead to
> > memory corruption.  For example, with a negative value then we might not
> > allocate enough for sizeof(struct group_info).
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > v2:  In v1, I changed groups_alloc().  The other places which call
> > groups_alloc() check the value before calling.  Eric wanted that, either
> > have all the callers check, or all the callers rely on groups_alloc().
> > In the end, Bruce Fields said adding the check here was probably
> > reasonable.
> > 
> > diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> > index 224a82f..1095be9 100644
> > --- a/net/sunrpc/auth_gss/svcauth_gss.c
> > +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> > @@ -463,6 +463,8 @@ static int rsc_parse(struct cache_detail *cd,
> >  		/* number of additional gid's */
> >  		if (get_int(&mesg, &N))
> >  			goto out;
> > +		if (N < 0 || N > NGROUPS_MAX)
> > +			goto out;
> >  		status = -ENOMEM;
> >  		rsci.cred.cr_group_info = groups_alloc(N);
> >  		if (rsci.cred.cr_group_info == NULL)
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> I touched this code relatively recently, and this check looks correct.
> Feel free to add Reviewed-by: Simo Sorce <simo@redhat.com>

Thanks!  I thought your below-the-line context was useful, so pulled a
version of it into the commit.

--b.

commit 76cb4be993c0
Author: Dan Carpenter <dan.carpenter@oracle.com>
Date:   Tue Feb 24 18:34:01 2015 +0300

    sunrpc: integer underflow in rsc_parse()
    
    If we call groups_alloc() with invalid values then it's might lead to
    memory corruption.  For example, with a negative value then we might not
    allocate enough for sizeof(struct group_info).
    
    (We're doing this in the caller for consistency with other callers of
    groups_alloc().  The other alternative might be to move the check out of
    all the callers into groups_alloc().)
    
    Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
    Reviewed-by: Simo Sorce <simo@redhat.com>
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 224a82f24d3c..1095be9c80ab 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -463,6 +463,8 @@ static int rsc_parse(struct cache_detail *cd,
 		/* number of additional gid's */
 		if (get_int(&mesg, &N))
 			goto out;
+		if (N < 0 || N > NGROUPS_MAX)
+			goto out;
 		status = -ENOMEM;
 		rsci.cred.cr_group_info = groups_alloc(N);
 		if (rsci.cred.cr_group_info == NULL)