DF-2996 / fix.diff
--- a/sys/vfs/nfs/nfs_vnops.c +++ b/sys/vfs/nfs/nfs_vnops.c @@ -2991,9 +2991,17 @@ { struct sillyrename *sp; struct nfsnode *np; + struct nfsnode *np2; int error; /* + * np must be NULL on entry: nfs_lookitup() leaves *npp untouched + * when it fails, and a garbage non-NULL *npp makes it take the + * "update the file handle of this node" path on a wild pointer. + */ + np = NULL; + + /* * Force finalization so the VOP_INACTIVE() call is not delayed. * This prevents cred structures from building up in nfsnodes * for deleted files. @@ -3035,6 +3043,41 @@ goto bad; error = nfs_lookitup(dvp, sp->s_name, sp->s_namlen, sp->s_cred, cnp->cn_td, &np); + + /* + * The looked-up object MUST be the renamed file itself. A + * malicious (or buggy) server can return any file handle here - + * including a directory's - and nfs_lookitup() then hands us + * whatever nfsnode that handle maps to. Storing sp through the + * wrong np is a wild pointer write when the lookup failed, and a + * type confusion against the n_cookies LIST_HEAD when the node + * happens to be a directory (nfs_reclaim() would walk and free + * sp as an nfsdmap). + */ + if (error == 0) { + np2 = VTONFS(vp); + /* + * Wrong object, or the server's attributes retyped the + * node (e.g. VREG -> VDIR) while nfs_lookitup() applied + * them: either way n_sillyrename would alias n_cookies. + */ + if ((np != np2 && NFS_CMPFH(np, np2->n_fhp, np2->n_fhsize) == 0) + || NFSTOV(np)->v_type != VREG) { + /* wrong object: release what lookitup acquired */ + if (np == VTONFS(dvp)) { + vrele(dvp); + } else { + vput(NFSTOV(np)); + } + error = EBADRPC; + } + } + if (error) { + vrele(sp->s_dvp); + crfree(sp->s_cred); + kfree((caddr_t)sp, M_NFSREQ); + return (error); + } np->n_sillyrename = sp; return (0); bad: @@ -3085,6 +3128,7 @@ np->n_fhsize = fhlen; newvp = NFSTOV(np); } else if (NFS_CMPFH(dnp, nfhp, fhlen)) { + np = dnp; /* *npp must never stay uninitialized */ vref(dvp); newvp = dvp; } else { |