Date: Wed, 28 Jan 2004 18:04:21 +0000 (GMT)
From: Hugh Dickins <hugh@veritas.com>
To: Tim Hockin <thockin@hockin.org>
cc: Andrew Morton <akpm@osdl.org>, <thockin@sun.com>, <torvalds@osdl.org>,
       <linux-kernel@vger.kernel.org>, <rusty@rustcorp.com.au>
Subject: Re: NGROUPS 2.6.2rc2
In-Reply-To: <Pine.LNX.4.44.0401281706040.6069-100000@localhost.localdomain>
Message-ID: <Pine.LNX.4.44.0401281757190.6213-100000@localhost.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1254
Lines: 30

On Wed, 28 Jan 2004, Hugh Dickins wrote:
> On Tue, 27 Jan 2004, Tim Hockin wrote:
> > On Tue, Jan 27, 2004 at 04:46:15PM -0800, Andrew Morton wrote:
> > > +
> > > +	if (info->ngroups > TASK_SIZE/sizeof(group))
> > > +		return -EFAULT;
> > > +	if (!access_ok(VERIFY_WRITE, grouplist, info->ngroups * sizeof(group)))
> > > +		return -EFAULT;
> > > 
> > > Why are many functions playing with TASK_SIZE?
> > 
> > Not sure - I thought it was maybe a paranoid check, Rusty included it in his
> > version of a similar patch a while ago.
> 
> Yes, a necessary paranoid check: without it, info->ngroups * sizeof(group)
> can easily wrap to something small, and access_ok pass when it should fail.

Sorry, I should have looked further.  info->ngroups is an "int", so
if this check is needed, a check for negativity (or unsigned cast)
would also be needed.  But it shouldn't be needed in the copy to user
cases, and in the copy from user cases gidsetsize should be checked
much earlier, in or before groups_alloc.

Hugh

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/