Date: Tue, 27 Aug 2024 21:20:43 GMT From: Rick Macklem <rmacklem@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 5037c6398b23 - main - nfsd: Fix handling of NFSv4 setable attributes Message-ID: <202408272120.47RLKhBV027330@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by rmacklem: URL: https://cgit.FreeBSD.org/src/commit/?id=5037c6398b2327366494a0434a894dc17ba8d023 commit 5037c6398b2327366494a0434a894dc17ba8d023 Author: Rick Macklem <rmacklem@FreeBSD.org> AuthorDate: 2024-08-27 21:19:33 +0000 Commit: Rick Macklem <rmacklem@FreeBSD.org> CommitDate: 2024-08-27 21:19:33 +0000 nfsd: Fix handling of NFSv4 setable attributes Commit d8a5961 made a change to nfsv4_sattr() that broke parsing of the setable attributes for a NFSv4 SETATTR. (It broke out of the code by setting "error" and returning right away, instead of noting the error in nd_repstat and allowing parsing of the attributes to continue.) By returning prematurely, it was possible for SETATTR to return the error, but with a bogus set of attribute bits set, since "retbits" had not yet been set to all zeros. (I am not sure if any client could be affected by this bug. The patch was done for a failure case detected by a pynfs test suite and not an actual client.) While here, the patch also fixes a few cases where the value of attributes gets set for attributes after an error has been set in nd_repstat. This would not really break the protocol, since a SETATTR is allowed to set some attributes and still return an failure, but should not really be done. MFC after: 2 weeks --- sys/fs/nfsserver/nfs_nfsdport.c | 74 ++++++++++++++++++++++++----------------- sys/fs/nfsserver/nfs_nfsdserv.c | 2 +- 2 files changed, 44 insertions(+), 32 deletions(-) diff --git a/sys/fs/nfsserver/nfs_nfsdport.c b/sys/fs/nfsserver/nfs_nfsdport.c index 12592d4c5c59..8a2a39052506 100644 --- a/sys/fs/nfsserver/nfs_nfsdport.c +++ b/sys/fs/nfsserver/nfs_nfsdport.c @@ -3026,6 +3026,8 @@ nfsv4_sattr(struct nfsrv_descript *nd, vnode_t vp, struct nfsvattr *nvap, /* * Loop around getting the setable attributes. If an unsupported * one is found, set nd_repstat == NFSERR_ATTRNOTSUPP and return. + * Once nd_repstat != 0, do not set the attribute value, but keep + * parsing the attribute(s). */ if (retnotsup) { nd->nd_repstat = NFSERR_ATTRNOTSUPP; @@ -3043,12 +3045,13 @@ nfsv4_sattr(struct nfsrv_descript *nd, vnode_t vp, struct nfsvattr *nvap, switch (bitpos) { case NFSATTRBIT_SIZE: NFSM_DISSECT(tl, u_int32_t *, NFSX_HYPER); - if (vp != NULL && vp->v_type != VREG) { - error = (vp->v_type == VDIR) ? NFSERR_ISDIR : - NFSERR_INVAL; - goto nfsmout; + if (!nd->nd_repstat) { + if (vp != NULL && vp->v_type != VREG) + nd->nd_repstat = (vp->v_type == VDIR) ? + NFSERR_ISDIR : NFSERR_INVAL; + else + nvap->na_size = fxdr_hyper(tl); } - nvap->na_size = fxdr_hyper(tl); attrsum += NFSX_HYPER; break; case NFSATTRBIT_ACL: @@ -3085,7 +3088,8 @@ nfsv4_sattr(struct nfsrv_descript *nd, vnode_t vp, struct nfsvattr *nvap, case NFSATTRBIT_MODE: moderet = NFSERR_INVAL; /* Can't do MODESETMASKED. */ NFSM_DISSECT(tl, u_int32_t *, NFSX_UNSIGNED); - nvap->na_mode = nfstov_mode(*tl); + if (!nd->nd_repstat) + nvap->na_mode = nfstov_mode(*tl); attrsum += NFSX_UNSIGNED; break; case NFSATTRBIT_OWNER: @@ -3153,10 +3157,11 @@ nfsv4_sattr(struct nfsrv_descript *nd, vnode_t vp, struct nfsvattr *nvap, attrsum += NFSX_UNSIGNED; if (fxdr_unsigned(int, *tl)==NFSV4SATTRTIME_TOCLIENT) { NFSM_DISSECT(tl, u_int32_t *, NFSX_V4TIME); - fxdr_nfsv4time(tl, &nvap->na_atime); + if (!nd->nd_repstat) + fxdr_nfsv4time(tl, &nvap->na_atime); toclient = 1; attrsum += NFSX_V4TIME; - } else { + } else if (!nd->nd_repstat) { vfs_timestamp(&nvap->na_atime); nvap->na_vaflags |= VA_UTIMES_NULL; } @@ -3169,7 +3174,8 @@ nfsv4_sattr(struct nfsrv_descript *nd, vnode_t vp, struct nfsvattr *nvap, break; case NFSATTRBIT_TIMECREATE: NFSM_DISSECT(tl, u_int32_t *, NFSX_V4TIME); - fxdr_nfsv4time(tl, &nvap->na_btime); + if (!nd->nd_repstat) + fxdr_nfsv4time(tl, &nvap->na_btime); attrsum += NFSX_V4TIME; break; case NFSATTRBIT_TIMEMODIFYSET: @@ -3177,10 +3183,11 @@ nfsv4_sattr(struct nfsrv_descript *nd, vnode_t vp, struct nfsvattr *nvap, attrsum += NFSX_UNSIGNED; if (fxdr_unsigned(int, *tl)==NFSV4SATTRTIME_TOCLIENT) { NFSM_DISSECT(tl, u_int32_t *, NFSX_V4TIME); - fxdr_nfsv4time(tl, &nvap->na_mtime); + if (!nd->nd_repstat) + fxdr_nfsv4time(tl, &nvap->na_mtime); nvap->na_vaflags &= ~VA_UTIMES_NULL; attrsum += NFSX_V4TIME; - } else { + } else if (!nd->nd_repstat) { vfs_timestamp(&nvap->na_mtime); if (!toclient) nvap->na_vaflags |= VA_UTIMES_NULL; @@ -3198,18 +3205,21 @@ nfsv4_sattr(struct nfsrv_descript *nd, vnode_t vp, struct nfsvattr *nvap, * specified and this attribute cannot be done in the * same Setattr operation. */ - if ((nd->nd_flag & ND_NFSV41) == 0) - nd->nd_repstat = NFSERR_ATTRNOTSUPP; - else if ((mode & ~07777) != 0 || (mask & ~07777) != 0 || - vp == NULL) - nd->nd_repstat = NFSERR_INVAL; - else if (moderet == 0) - moderet = VOP_GETATTR(vp, &va, nd->nd_cred); - if (moderet == 0) - nvap->na_mode = (mode & mask) | - (va.va_mode & ~mask); - else - nd->nd_repstat = moderet; + if (!nd->nd_repstat) { + if ((nd->nd_flag & ND_NFSV41) == 0) + nd->nd_repstat = NFSERR_ATTRNOTSUPP; + else if ((mode & ~07777) != 0 || + (mask & ~07777) != 0 || vp == NULL) + nd->nd_repstat = NFSERR_INVAL; + else if (moderet == 0) + moderet = VOP_GETATTR(vp, &va, + nd->nd_cred); + if (moderet == 0) + nvap->na_mode = (mode & mask) | + (va.va_mode & ~mask); + else + nd->nd_repstat = moderet; + } attrsum += 2 * NFSX_UNSIGNED; break; case NFSATTRBIT_MODEUMASK: @@ -3220,13 +3230,15 @@ nfsv4_sattr(struct nfsrv_descript *nd, vnode_t vp, struct nfsvattr *nvap, * If moderet != 0, mode has already been done. * If vp != NULL, this is not a file object creation. */ - if ((nd->nd_flag & ND_NFSV42) == 0) - nd->nd_repstat = NFSERR_ATTRNOTSUPP; - else if ((mask & ~0777) != 0 || vp != NULL || - moderet != 0) - nd->nd_repstat = NFSERR_INVAL; - else - nvap->na_mode = (mode & ~mask); + if (!nd->nd_repstat) { + if ((nd->nd_flag & ND_NFSV42) == 0) + nd->nd_repstat = NFSERR_ATTRNOTSUPP; + else if ((mask & ~0777) != 0 || vp != NULL || + moderet != 0) + nd->nd_repstat = NFSERR_INVAL; + else + nvap->na_mode = (mode & ~mask); + } attrsum += 2 * NFSX_UNSIGNED; break; default: @@ -3241,7 +3253,7 @@ nfsv4_sattr(struct nfsrv_descript *nd, vnode_t vp, struct nfsvattr *nvap, /* * some clients pad the attrlist, so we need to skip over the - * padding. + * padding. This also skips over unparsed non-supported attributes. */ if (attrsum > attrsize) { error = NFSERR_BADXDR; diff --git a/sys/fs/nfsserver/nfs_nfsdserv.c b/sys/fs/nfsserver/nfs_nfsdserv.c index 47e3a20390f4..a38ef3d47946 100644 --- a/sys/fs/nfsserver/nfs_nfsdserv.c +++ b/sys/fs/nfsserver/nfs_nfsdserv.c @@ -375,6 +375,7 @@ nfsrvd_setattr(struct nfsrv_descript *nd, __unused int isdgram, NFSACL_T *aclp = NULL; struct thread *p = curthread; + NFSZERO_ATTRBIT(&retbits); if (nd->nd_repstat) { nfsrv_wcc(nd, preat_ret, &nva2, postat_ret, &nva); goto out; @@ -402,7 +403,6 @@ nfsrvd_setattr(struct nfsrv_descript *nd, __unused int isdgram, goto nfsmout; /* For NFSv4, only va_uid is used from nva2. */ - NFSZERO_ATTRBIT(&retbits); NFSSETBIT_ATTRBIT(&retbits, NFSATTRBIT_OWNER); preat_ret = nfsvno_getattr(vp, &nva2, nd, p, 1, &retbits); if (!nd->nd_repstat)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202408272120.47RLKhBV027330>