On Mon, 2024-06-10 at 11:04 -0400, cel@xxxxxxxxxx wrote:
> From: Chuck Lever <chuck.lever@xxxxxxxxxx>
>
> I noticed LAYOUTGET(LAYOUTIOMODE4_RW) returning NFS4ERR_ACCESS
> unexpectedly. The NFS client had created a file with mode 0444, and
> the server had returned a write delegation on the OPEN(CREATE). The
> client was requesting a RW layout using the write delegation stateid
> so that it could flush file modifications.
>
> This client behavior was permitted for NFSv4.1 without pNFS, so I
> began looking at NFSD's implementation of LAYOUTGET.
>
> The failure was because fh_verify() was doing a permission check as
> part of verifying the FH. It uses the loga_iomode value to specify
> the @accmode argument. fh_verify(MAY_WRITE) on a file whose mode is
> 0444 fails with -EACCES.
>
> RFC 8881 Section 18.43.3 states:
> > The use of the loga_iomode field depends upon the layout type, but
> > should reflect the client's data access intent.
>
> Further discussion of iomode values focuses on how the server is
> permitted to change returned the iomode when coalescing layouts.
> It says nothing about mandating the denial of LAYOUTGET requests
> due to file permission settings.
>
> Appropriate permission checking is done when the client acquires the
> stateid used in the LAYOUTGET operation, so remove the permission
> check from LAYOUTGET and LAYOUTCOMMIT, and rely on layout stateid
> checking instead.
Hmm... I'm not seeing any checking or enforcement of the
EXCHGID4_FLAG_BIND_PRINC_STATEID flag in knfsd. Doesn't that mean that
READ and WRITE are required to check access permissions, as per
RFC8881, section 13.9.2.3?
I'm not saying that the return of an ACCESS error is correct here,
since the file was created using this credential, and so the caller
should benefit from having owner privileges.
However the issue is that knfsd should be either checking that the
credential is correct w.r.t. the stateid (if
EXCHGID4_FLAG_BIND_PRINC_STATEID is set and the stateid is not a
special stateid) or that it is correct w.r.t. the file permissions (if
EXCHGID4_FLAG_BIND_PRINC_STATEID is not set or the stateid is a special
stateid).
>
> Cc: Christoph Hellwig <hch@xxxxxx>
> X-Cc: stable@xxxxxxxxxxxxxxx # v6.6+
> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> ---
> fs/nfsd/nfs4proc.c | 12 +++---------
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 46bd20fe5c0f..c24f45870b28 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -2269,23 +2269,17 @@ nfsd4_layoutget(struct svc_rqst *rqstp,
> const struct nfsd4_layout_ops *ops;
> struct nfs4_layout_stateid *ls;
> __be32 nfserr;
> - int accmode = NFSD_MAY_READ_IF_EXEC;
>
> + nfserr = nfserr_badiomode;
> switch (lgp->lg_seg.iomode) {
> case IOMODE_READ:
> - accmode |= NFSD_MAY_READ;
> - break;
> case IOMODE_RW:
> - accmode |= NFSD_MAY_READ | NFSD_MAY_WRITE;
> break;
> default:
> - dprintk("%s: invalid iomode %d\n",
> - __func__, lgp->lg_seg.iomode);
> - nfserr = nfserr_badiomode;
> goto out;
> }
>
> - nfserr = fh_verify(rqstp, current_fh, 0, accmode);
> + nfserr = fh_verify(rqstp, current_fh, 0, NFSD_MAY_NOP);
> if (nfserr)
> goto out;
>
> @@ -2359,7 +2353,7 @@ nfsd4_layoutcommit(struct svc_rqst *rqstp,
> struct nfs4_layout_stateid *ls;
> __be32 nfserr;
>
> - nfserr = fh_verify(rqstp, current_fh, 0, NFSD_MAY_WRITE);
> + nfserr = fh_verify(rqstp, current_fh, 0, NFSD_MAY_NOP);
> if (nfserr)
> goto out;
>
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
