On Wed, 16 Apr 2008 14:37:28 -0400
"J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote:
> On Wed, Apr 16, 2008 at 08:38:51AM -0400, Jeff Layton wrote:
> > Currently, knfsd only clears the setuid bit if the owner of a file
> > is changed on a SETATTR call, and only clears the setgid bit if
> > the group is changed. POSIX says this in the spec for chown():
> >
> > "If the specified file is a regular file, one or more of the
> > S_IXUSR, S_IXGRP, or S_IXOTH bits of the file mode are set, and the
> > process does not have appropriate privileges, the set-user-ID
> > (S_ISUID) and set-group-ID (S_ISGID) bits of the file mode shall
> > be cleared upon successful return from chown()."
> >
> > If I'm reading this correct, then knfsd is doing this wrong. It should
> > be clearing both the setuid and setgid bit on any SETATTR that changes
> > the uid or gid. This wasn't really as noticable before, but now that the
> > ATTR_KILL_S*ID bits are a no-op for the NFS client, it's more evident.
>
> OK, thanks!
>
> >
> > This patch corrects the nfsd_setattr logic so that this occurs. It also
> > does a bit of cleanup to the function and eliminates an unneeded local
> > variable.
>
> Might be nice to have the cleanup separately first, if that's easy.
> (Should be for the "imode" removal, at least. Wonder how that got
> there?)
>
Yep, that's the main thing I was cleaning up. I'll plan to respin it as
2 or more separate patches.
> One other question:
>
> >
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > fs/nfsd/vfs.c | 30 ++++++++++++++----------------
> > 1 files changed, 14 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 17ac51b..7d6cd1a 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -264,7 +264,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> > struct inode *inode;
> > int accmode = MAY_SATTR;
> > int ftype = 0;
> > - int imode;
> > __be32 err;
> > int host_err;
> > int size_change = 0;
> > @@ -360,25 +359,24 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> > DQUOT_INIT(inode);
> > }
> >
> > - imode = inode->i_mode;
> > if (iap->ia_valid & ATTR_MODE) {
> > iap->ia_mode &= S_IALLUGO;
> > - imode = iap->ia_mode |= (imode & ~S_IALLUGO);
> > - /* if changing uid/gid revoke setuid/setgid in mode */
> > - if ((iap->ia_valid & ATTR_UID) && iap->ia_uid != inode->i_uid) {
> > - iap->ia_valid |= ATTR_KILL_PRIV;
> > + iap->ia_mode |= (inode->i_mode & ~S_IALLUGO);
> > + }
> > +
> > + /* Revoke setuid/setgid on chown */
> > + if (((iap->ia_valid & ATTR_UID) && iap->ia_uid != inode->i_uid) ||
> > + ((iap->ia_valid & ATTR_GID) && iap->ia_gid != inode->i_gid)) {
> > + iap->ia_valid |= ATTR_KILL_PRIV;
> > + if (iap->ia_valid & ATTR_MODE) {
> > + /* we're setting mode too, just clear the mode bits */
> > iap->ia_mode &= ~S_ISUID;
> > + if (iap->ia_mode & S_IXGRP)
> > + iap->ia_mode &= ~S_ISGID;
>
> Why the check for S_IXGRP? I notice it's mentioned in the posix text
> (though this doesn't implement precisely that suggestion), but the old
> code didn't do it that I can see.
>
> --b.
>
Yes, that is different. I tried to mirror what notify_change actually
does. It's possible that someone could set the S_ISGID bit without
S_IXGRP, as that signifies mandatory locking. In that case, we
probably don't want to clear the S_ISGID bit.
> > + } else {
> > + /* set ATTR_KILL_* bits and let VFS handle it */
> > + iap->ia_valid |= (ATTR_KILL_SUID | ATTR_KILL_SGID);
> > }
> > - if ((iap->ia_valid & ATTR_GID) && iap->ia_gid != inode->i_gid)
> > - iap->ia_mode &= ~S_ISGID;
> > - } else {
> > - /*
> > - * Revoke setuid/setgid bit on chown/chgrp
> > - */
> > - if ((iap->ia_valid & ATTR_UID) && iap->ia_uid != inode->i_uid)
> > - iap->ia_valid |= ATTR_KILL_SUID | ATTR_KILL_PRIV;
> > - if ((iap->ia_valid & ATTR_GID) && iap->ia_gid != inode->i_gid)
> > - iap->ia_valid |= ATTR_KILL_SGID;
> > }
> >
> > /* Change the attributes. */
> > --
> > 1.5.3.6
> >
Thanks!
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
