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 74e7fed..05699b8 100644 (file)
--- 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);