Return-Path: Received: from fieldses.org ([173.255.197.46]:49442 "EHLO fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752154AbcGVB5Y (ORCPT ); Thu, 21 Jul 2016 21:57:24 -0400 Date: Thu, 21 Jul 2016 21:57:22 -0400 From: "J. Bruce Fields" To: Oleg Drokin Cc: Jeff Layton , linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] nfsd: Make creates return EEXIST correctly instead of EPERM Message-ID: <20160722015722.GA29969@fieldses.org> References: <1467942466-3081422-1-git-send-email-green@linuxhacker.ru> <20160708205413.GC11269@fieldses.org> <20160721203415.GE27148@fieldses.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: Sender: linux-nfs-owner@vger.kernel.org List-ID: On Thu, Jul 21, 2016 at 04:37:40PM -0400, Oleg Drokin wrote: > > On Jul 21, 2016, at 4:34 PM, J. Bruce Fields wrote: > > > On Fri, Jul 08, 2016 at 05:53:19PM -0400, Oleg Drokin wrote: > >> > >> On Jul 8, 2016, at 4:54 PM, J. Bruce Fields wrote: > >> > >>> On Thu, Jul 07, 2016 at 09:47:46PM -0400, Oleg Drokin wrote: > >>>> It looks like we are bit overzealous about failing mkdir/create/mknod > >>>> with permission denied if the parent dir is not writeable. > >>>> Need to make sure the name does not exist first, because we need to > >>>> return EEXIST in that case. > >>>> > >>>> Signed-off-by: Oleg Drokin > >>>> --- > >>>> A very similar problem exists with symlinks, but the patch is more > >>>> involved, so assuming this one is ok, I'll send a symlink one separately. > >>>> fs/nfsd/nfs4proc.c | 6 +++++- > >>>> fs/nfsd/vfs.c | 11 ++++++++++- > >>>> 2 files changed, 15 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > >>>> index de1ff1d..0067520 100644 > >>>> --- a/fs/nfsd/nfs4proc.c > >>>> +++ b/fs/nfsd/nfs4proc.c > >>>> @@ -605,8 +605,12 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > >>>> > >>>> fh_init(&resfh, NFS4_FHSIZE); > >>>> > >>>> + /* > >>>> + * We just check thta parent is accessible here, nfsd_* do their > >>>> + * own access permission checks > >>>> + */ > >>>> status = fh_verify(rqstp, &cstate->current_fh, S_IFDIR, > >>>> - NFSD_MAY_CREATE); > >>>> + NFSD_MAY_EXEC); > >>>> if (status) > >>>> return status; > >>>> > >>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > >>>> index 6fbd81e..6a45ec6 100644 > >>>> --- a/fs/nfsd/vfs.c > >>>> +++ b/fs/nfsd/vfs.c > >>>> @@ -1161,7 +1161,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp, > >>>> if (isdotent(fname, flen)) > >>>> goto out; > >>>> > >>>> - err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE); > >>>> + /* > >>>> + * Even though it is a create, first we see if we are even allowed > >>>> + * to peek inside the parent > >>>> + */ > >>>> + err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_EXEC); > >>> > >>> Looks like in the v3 case we haven't actually locked the directory yet > >>> at this point so this check is a little race-prone. > >> > >> In reality this check is not really needed, I suspect. > >> When we call vfs_create/mknod/mkdir later on, it has it's own permission check > >> anyway so if there was a race and somebody changed dir access in the middle, > >> there's going to be another check anyway and it would be caught. > >> Unless there's some weird server-side permission wiggling as well that makes it > >> ineffective, but I imagine that one cannot really change in a racy way? > > > > Yeah, I think I'll just change those NFSD_MAY_EXEC's to NFSD_MAY_NOP's. > > We still need the fh_verify there since it's also what does the > > filehandle->dentry translation, but we don't need permission checking > > here yet. > > This will likely need an extra test to ensure that when you > do mkdir where you do not have exec permissions, you would get EACCES instead > of EEXIST, otherwise that would be information leakage, no? > Or do you think the second time we do nfsd_permission, that would be covered? No, you're right, for some reason I thought that the check for a positive inode didn't happen till later. But actually the logic is basically: lock inode lookup_one_len return nfserr_exist if looked up dentry is positive. check for create permission vfs_create So, yes, the initial MAY_EXEC test's needed to prevent that information leak. That said... I wonder why it's done that way? Seems to me we could just tremove that nfserr_exist check and the vfs would handle it for us.... I'll try that. --b. > > > Applying with that one change. (And I'll followup with some additional > > minor cleanup of the create code.) > > > > --b. > > > >> > >>> I wonder why the code's structured that way--it's confusing. > >> > >> Probably years of accumulated "damage" ;) > >> > >>> --b. > >>> > >>>> if (err) > >>>> goto out; > >>>> > >>>> @@ -1211,6 +1215,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp, > >>>> goto out; > >>>> } > >>>> > >>>> + /* Now let's see if we actually have permissions to create */ > >>>> + err = nfsd_permission(rqstp, fhp->fh_export, dentry, NFSD_MAY_CREATE); > >>>> + if (err) > >>>> + goto out; > >>>> + > >>>> if (!(iap->ia_valid & ATTR_MODE)) > >>>> iap->ia_mode = 0; > >>>> iap->ia_mode = (iap->ia_mode & S_IALLUGO) | type; > >>>> -- > >>>> 2.7.4