On Mon, Jul 11, 2016 at 05:06:33PM -0400, Ted Unangst wrote:
> Todd C. Miller wrote:
> > On Mon, 11 Jul 2016 16:39:05 -0400, "Ted Unangst" wrote:
> >
> > > sigh. i don't know what else can trigger that kassert, so just fix the
> > > caller
> > > to do the same check and return an error.
> >
> > Checking for VNOVAL is kind of bogus. How about we try something
> > more sensible?
>
> those checks are equally useless. UID_MAX is UINT_MAX so the tests don't fire.
>
> the question is what other tmpfs code blows up when nodes owned by -1 start
> showing up.
The check for VNOVAl is probably the right thing to do here. Using
VNOVAL to indicate the absence of a value is an established (although
rather questionable) practice in filesystem code. e.g. a VOP_SETATTR()
call with vap->va_uid set to VNOVAL will not chown the file, making
VNOVAL a de facto illegal value for a file owner.
>
> >
> > - todd
> >
> > Index: tmpfs_subr.c
> > ===================================================================
> > RCS file: /cvs/src/sys/tmpfs/tmpfs_subr.c,v
> > retrieving revision 1.16
> > diff -u -p -u -r1.16 tmpfs_subr.c
> > --- tmpfs_subr.c 19 Jun 2016 11:54:33 -0000 1.16
> > +++ tmpfs_subr.c 11 Jul 2016 20:45:26 -0000
> > @@ -139,8 +139,7 @@ tmpfs_alloc_node(tmpfs_mount_t *tmp, enu
> > nnode->tn_ctime = nnode->tn_atime;
> > nnode->tn_mtime = nnode->tn_atime;
> >
> > - /* XXX pedro: we should check for UID_MAX and GID_MAX instead. */
> > - KASSERT(uid != VNOVAL && gid != VNOVAL && mode != VNOVAL);
> > + KASSERT(uid <= UID_MAX && gid <= GID_MAX && (mode & ALLPERMS) == mode);
> >
> > nnode->tn_uid = uid;
> > nnode->tn_gid = gid;
> > Index: tmpfs_vfsops.c
> > ===================================================================
> > RCS file: /cvs/src/sys/tmpfs/tmpfs_vfsops.c,v
> > retrieving revision 1.8
> > diff -u -p -u -r1.8 tmpfs_vfsops.c
> > --- tmpfs_vfsops.c 13 Jan 2016 13:01:40 -0000 1.8
> > +++ tmpfs_vfsops.c 11 Jul 2016 20:45:20 -0000
> > @@ -126,6 +126,11 @@ tmpfs_mount(struct mount *mp, const char
> > if (error)
> > return error;
> >
> > + /* Sanity check args. */
> > + if (args.ta_root_uid > UID_MAX || args.ta_root_gid > GID_MAX ||
> > + (args.ta_root_mode & ALLPERMS) != args.ta_root_mode)
> > + return EINVAL;
> > +
> > /* Get the memory usage limit for this file-system. */
> > if (args.ta_size_max < PAGE_SIZE) {
> > memlimit = UINT64_MAX;
> >
>
