| Message ID | 20150223154419.GA2542@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; -- 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
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 -- 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
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 -- 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
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. -- 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
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;
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. -- 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