From: miod Date: Tue, 10 Sep 2024 18:44:04 +0000 (+0000) Subject: nfsm_srvnamesiz() may set up an NFSERR_NAMETOL error, which nfsm_reply() would X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=8e9d9dd5c33e0e030dfe90398bf77e117da89540;p=openbsd nfsm_srvnamesiz() may set up an NFSERR_NAMETOL error, which nfsm_reply() would consider as not tragic enough to abort the operation, in order to batch error replies. This would end up invoking nfs_namei() using an uninitialized variable as length, and Bad Things(TM) would happen. Reported by Claes M Nyberg on bugs@; tweaks & ok claudio@ --- diff --git a/sys/nfs/nfs_serv.c b/sys/nfs/nfs_serv.c index 74e7fed74e4..05699b8dd1a 100644 --- a/sys/nfs/nfs_serv.c +++ b/sys/nfs/nfs_serv.c @@ -1,4 +1,4 @@ -/* $OpenBSD: nfs_serv.c,v 1.127 2024/08/17 07:02:13 jsg Exp $ */ +/* $OpenBSD: nfs_serv.c,v 1.128 2024/09/10 18:44:04 miod Exp $ */ /* $NetBSD: nfs_serv.c,v 1.34 1997/05/12 23:37:12 fvdl Exp $ */ /* @@ -424,11 +424,13 @@ nfsm_srvnamesiz(struct nfsm_info *infop, int *lenp) if (tl == NULL) return 1; len = fxdr_unsigned(int32_t, *tl); - if (len > NFS_MAXNAMLEN) + if (len > NFS_MAXNAMLEN) { *infop->nmi_errorp = NFSERR_NAMETOL; - else if (len <= 0) + *lenp = 0; + } else if (len <= 0) { *infop->nmi_errorp = EBADRPC; - else { + *lenp = 0; + } else { *infop->nmi_errorp = 0; *lenp = len; } @@ -471,8 +473,13 @@ nfsrv_lookup(struct nfsrv_descript *nfsd, struct nfssvc_sock *slp, if (nfsm_srvnamesiz(&info, &len) != 0) goto nfsmout; if (error) { - if (nfsm_reply(&info, nfsd, slp, mrq, error, 0) != 0) - return 0; + /* + * nfsm_reply would return zero if v3 and an error different + * from EBADRPC. But it does not make sense to continue + * anyway if the error set in nfsm_srvnamesiz is NFSERR_NAMETOL. + */ + (void)nfsm_reply(&info, nfsd, slp, mrq, error, 0); + return 0; } NDINIT(&nd, LOOKUP, LOCKLEAF | SAVESTART, UIO_SYSSPACE, NULL, procp); @@ -1045,8 +1052,13 @@ nfsrv_create(struct nfsrv_descript *nfsd, struct nfssvc_sock *slp, if (nfsm_srvnamesiz(&info, &len) != 0) return error; if (error) { - if (nfsm_reply(&info, nfsd, slp, mrq, error, 0) != 0) - return 0; + /* + * nfsm_reply would return zero if v3 and an error different + * from EBADRPC. But it does not make sense to continue + * anyway if the error set in nfsm_srvnamesiz is NFSERR_NAMETOL. + */ + (void)nfsm_reply(&info, nfsd, slp, mrq, error, 0); + return 0; } NDINIT(&nd, CREATE, LOCKPARENT | LOCKLEAF | SAVESTART, UIO_SYSSPACE, @@ -1332,8 +1344,13 @@ nfsrv_mknod(struct nfsrv_descript *nfsd, struct nfssvc_sock *slp, if (nfsm_srvnamesiz(&info, &len) != 0) return error; if (error) { - if (nfsm_reply(&info, nfsd, slp, mrq, error, 0) != 0) - return 0; + /* + * nfsm_reply would return zero if v3 and an error different + * from EBADRPC. But it does not make sense to continue + * anyway if the error set in nfsm_srvnamesiz is NFSERR_NAMETOL. + */ + (void)nfsm_reply(&info, nfsd, slp, mrq, error, 0); + return 0; } NDINIT(&nd, CREATE, LOCKPARENT | LOCKLEAF | SAVESTART, UIO_SYSSPACE, @@ -1509,8 +1526,13 @@ nfsrv_remove(struct nfsrv_descript *nfsd, struct nfssvc_sock *slp, if (nfsm_srvnamesiz(&info, &len) != 0) goto nfsmout; if (error) { - if (nfsm_reply(&info, nfsd, slp, mrq, error, 0) != 0) - return 0; + /* + * nfsm_reply would return zero if v3 and an error different + * from EBADRPC. But it does not make sense to continue + * anyway if the error set in nfsm_srvnamesiz is NFSERR_NAMETOL. + */ + (void)nfsm_reply(&info, nfsd, slp, mrq, error, 0); + return 0; } NDINIT(&nd, DELETE, LOCKPARENT | LOCKLEAF, UIO_SYSSPACE, NULL, procp); @@ -1605,8 +1627,13 @@ nfsrv_rename(struct nfsrv_descript *nfsd, struct nfssvc_sock *slp, if (nfsm_srvnamesiz(&info, &len) != 0) return error; if (error) { - if (nfsm_reply(&info, nfsd, slp, mrq, error, 0) != 0) - return 0; + /* + * nfsm_reply would return zero if v3 and an error different + * from EBADRPC. But it does not make sense to continue + * anyway if the error set in nfsm_srvnamesiz is NFSERR_NAMETOL. + */ + (void)nfsm_reply(&info, nfsd, slp, mrq, error, 0); + return 0; } /* @@ -1816,8 +1843,13 @@ nfsrv_link(struct nfsrv_descript *nfsd, struct nfssvc_sock *slp, if (nfsm_srvnamesiz(&info, &len) != 0) goto nfsmout; if (error) { - if (nfsm_reply(&info, nfsd, slp, mrq, error, 0) != 0) - return 0; + /* + * nfsm_reply would return zero if v3 and an error different + * from EBADRPC. But it does not make sense to continue + * anyway if the error set in nfsm_srvnamesiz is NFSERR_NAMETOL. + */ + (void)nfsm_reply(&info, nfsd, slp, mrq, error, 0); + return 0; } error = nfsrv_fhtovp(fhp, 0, &vp, cred, slp, nam, &rdonly); @@ -1929,8 +1961,13 @@ nfsrv_symlink(struct nfsrv_descript *nfsd, struct nfssvc_sock *slp, if (nfsm_srvnamesiz(&info, &len) != 0) return error; if (error) { - if (nfsm_reply(&info, nfsd, slp, mrq, error, 0) != 0) - return 0; + /* + * nfsm_reply would return zero if v3 and an error different + * from EBADRPC. But it does not make sense to continue + * anyway if the error set in nfsm_srvnamesiz is NFSERR_NAMETOL. + */ + (void)nfsm_reply(&info, nfsd, slp, mrq, error, 0); + return 0; } NDINIT(&nd, CREATE, LOCKPARENT | SAVESTART, UIO_SYSSPACE, NULL, procp); @@ -2089,8 +2126,13 @@ nfsrv_mkdir(struct nfsrv_descript *nfsd, struct nfssvc_sock *slp, if (nfsm_srvnamesiz(&info, &len) != 0) return error; if (error) { - if (nfsm_reply(&info, nfsd, slp, mrq, error, 0) != 0) - return 0; + /* + * nfsm_reply would return zero if v3 and an error different + * from EBADRPC. But it does not make sense to continue + * anyway if the error set in nfsm_srvnamesiz is NFSERR_NAMETOL. + */ + (void)nfsm_reply(&info, nfsd, slp, mrq, error, 0); + return 0; } NDINIT(&nd, CREATE, LOCKPARENT, UIO_SYSSPACE, NULL, procp); @@ -2218,8 +2260,13 @@ nfsrv_rmdir(struct nfsrv_descript *nfsd, struct nfssvc_sock *slp, if (nfsm_srvnamesiz(&info, &len) != 0) goto nfsmout; if (error) { - if (nfsm_reply(&info, nfsd, slp, mrq, error, 0) != 0) - return 0; + /* + * nfsm_reply would return zero if v3 and an error different + * from EBADRPC. But it does not make sense to continue + * anyway if the error set in nfsm_srvnamesiz is NFSERR_NAMETOL. + */ + (void)nfsm_reply(&info, nfsd, slp, mrq, error, 0); + return 0; } NDINIT(&nd, DELETE, LOCKPARENT | LOCKLEAF, UIO_SYSSPACE, NULL, procp);