2008-05-05 17:47:28

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] exportfs: fix incorrect EACCES in reconnect_path()

On Mon, May 05, 2008 at 09:22:49AM +1000, Neil Brown wrote:
> On Friday May 2, [email protected] wrote:
> > On Fri, May 02, 2008 at 12:04:53PM -0400, Trond Myklebust wrote:
> > >
> > > AFAICS, the real problem here is that nfsd is dropping its privileged
> > > mode too early. Why can't you call reconnect_path() using nfsd's root
> > > permissions instead of dropping permissions checks altogether?
> >
> > That's an interesting idea.
> >
> > As I understand it, nfsd sets the current task's credentials only once,
> > in nfsd_setuser, called from fh_verify(). The change lingers around
> > until next time we do fh_verify(). So in addition to moving the
> > nfsd_setuser() call to after the lookup of the dentry (so after
> > exportfs_decode_fh(), we'd also need to add an explicit acquisition of
> > whatever permissions we need before we do that lookup.
>
> I have substantial sympathy for this approach.
> The "explicit acquisition" would probably just be
> current->cap_effective =
> cap_raise_nfsd_set(current->cap_effective,
> current->cap_permitted);
> (from nfsd_setuser). This should reclaim the RACOVERRIDE permission
> which should be enough.
>
> The one problem that I see is that it would invalidate the
> err = permission(parent->d_inode, MAY_EXEC, NULL);
> check in nfsd_acceptable.
>
> Currently if we have "subtree_check" set, not only must the file be in
> the right subtree of the filesystem, but the user accessing the file
> must have 'x' permission on every parent directory. For that test to
> work we must have already set the effective uid correctly.
>
> Now it could be argued that this permission test is really a dumb idea
> that buys nothing and costs much. And if you were to queue a patch to
> get rid of it, I doubt you would get any objections .... certainly not
> from me :-)

Dumb idea or not, it looks like it's explicitly documented in
exports(5):

" subtree checking is also used to make sure that files
inside directories to which only root has access can only be
accessed if the filesystem is exported with no_root_squash
(see below), even if the file itself allows more general
access."

So as much as I'd like to I'm not comfortable silently turning off that
check.

I suppose we could choose to acquire those capabilities only in the
no_subtree_check case.

--b.