fix access-after-free bugs in dircache code by refcounting nfsdircache.

PR/26864.

sys/nfs/nfs_bio.c

@@ -1,4 +1,4 @@
-/*	$NetBSD: nfs_bio.c,v 1.119 2004/07/18 07:43:00 yamt Exp $	*/
+/*	$NetBSD: nfs_bio.c,v 1.120 2004/09/15 09:50:56 yamt Exp $	*/
 
 /*
  * Copyright (c) 1989, 1993
@@ -35,7 +35,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: nfs_bio.c,v 1.119 2004/07/18 07:43:00 yamt Exp $");
+__KERNEL_RCSID(0, "$NetBSD: nfs_bio.c,v 1.120 2004/09/15 09:50:56 yamt Exp $");
 
 #include "opt_nfs.h"
 #include "opt_ddb.h"
@@ -282,6 +282,7 @@ diragain:
 
 		if (uio->uio_offset != 0 &&
 		    ndp->dc_cookie == np->n_direofoffset) {
+			nfs_putdircache(np, ndp);
 			nfsstats.direofcache_hits++;
 			return (0);
 		}
@@ -299,6 +300,7 @@ diragain:
 			 * server. Punt and let the userland code
 			 * deal with it.
 			 */
+			nfs_putdircache(np, ndp);
 			brelse(bp);
 			if (error == NFSERR_BAD_COOKIE) {
 			    nfs_invaldircache(vp, 0);
@@ -317,6 +319,7 @@ diragain:
 		 */
 		if (np->n_direofoffset != 0 && 
 			ndp->dc_blkcookie == np->n_direofoffset) {
+			nfs_putdircache(np, ndp);
 			brelse(bp);
 			return (0);
 		}
@@ -351,6 +354,7 @@ diragain:
 				(unsigned long)uio->uio_offset,
 				(unsigned long)NFS_GETCOOKIE(pdp));
 #endif
+			nfs_putdircache(np, ndp);
 			brelse(bp);
 			nfs_invaldircache(vp, 0);
 			nfs_vinvalbuf(vp, 0, cred, p, 0);
@@ -393,11 +397,13 @@ diragain:
 					NFS_STASHCOOKIE32(pdp,
 					    nndp->dc_cookie32);
 				}
+				nfs_putdircache(np, nndp);
 			}
 			pdp = dp;
 			dp = (struct dirent *)((caddr_t)dp + dp->d_reclen);
 			enn++;
 		}
+		nfs_putdircache(np, ndp);
 
 		/*
 		 * If the last requested entry was not the last in the
@@ -414,6 +420,7 @@ diragain:
 				NFS_STASHCOOKIE32(pdp, nndp->dc_cookie32);
 				curoff = nndp->dc_cookie32;
 			}
+			nfs_putdircache(np, nndp);
 		} else
 			curoff = bp->b_dcookie;
 
@@ -452,6 +459,7 @@ diragain:
 				brelse(rabp);
 			}
 		}
+		nfs_putdircache(np, nndp);
 		got_buf = 1;
 		break;
 	    default:

sys/nfs/nfs_subs.c

@@ -1,4 +1,4 @@
-/*	$NetBSD: nfs_subs.c,v 1.134 2004/06/14 12:28:35 yamt Exp $	*/
+/*	$NetBSD: nfs_subs.c,v 1.135 2004/09/15 09:50:56 yamt Exp $	*/
 
 /*
  * Copyright (c) 1989, 1993
@@ -70,7 +70,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: nfs_subs.c,v 1.134 2004/06/14 12:28:35 yamt Exp $");
+__KERNEL_RCSID(0, "$NetBSD: nfs_subs.c,v 1.135 2004/09/15 09:50:56 yamt Exp $");
 
 #include "fs_nfs.h"
 #include "opt_nfs.h"
@@ -1214,19 +1214,32 @@ nfs_dirhash(off)
 	return sum;
 }
 
+#define	_NFSDC_MTX(np)		(&NFSTOV(np)->v_interlock)
+#define	NFSDC_LOCK(np)		simple_lock(_NFSDC_MTX(np))
+#define	NFSDC_UNLOCK(np)	simple_unlock(_NFSDC_MTX(np))
+#define	NFSDC_ASSERT_LOCKED(np) LOCK_ASSERT(simple_lock_held(_NFSDC_MTX(np)))
+
 void
 nfs_initdircache(vp)
 	struct vnode *vp;
 {
 	struct nfsnode *np = VTONFS(vp);
+	struct nfsdirhashhead *dircache;
 
-	KASSERT(np->n_dircache == NULL);
+	dircache = hashinit(NFS_DIRHASHSIZ, HASH_LIST, M_NFSDIROFF,
-
-	np->n_dircachesize = 0;
-	np->n_dblkno = 1;
-	np->n_dircache = hashinit(NFS_DIRHASHSIZ, HASH_LIST, M_NFSDIROFF,
 	    M_WAITOK, &nfsdirhashmask);
-	TAILQ_INIT(&np->n_dirchain);
+
+	NFSDC_LOCK(np);
+	if (np->n_dircache == NULL) {
+		np->n_dircachesize = 0;
+		np->n_dblkno = 1;
+		np->n_dircache = dircache;
+		dircache = NULL;
+		TAILQ_INIT(&np->n_dirchain);
+	}
+	NFSDC_UNLOCK(np);
+	if (dircache)
+		hashdone(dircache, M_NFSDIROFF);
 }
 
 void
@@ -1234,16 +1247,83 @@ nfs_initdirxlatecookie(vp)
 	struct vnode *vp;
 {
 	struct nfsnode *np = VTONFS(vp);
+	unsigned *dirgens;
 
 	KASSERT(VFSTONFS(vp->v_mount)->nm_flag & NFSMNT_XLATECOOKIE);
-	KASSERT(np->n_dirgens == NULL);
 
-	MALLOC(np->n_dirgens, unsigned *,
+	dirgens = malloc(NFS_DIRHASHSIZ * sizeof (unsigned), M_NFSDIROFF,
-	    NFS_DIRHASHSIZ * sizeof (unsigned), M_NFSDIROFF, M_WAITOK);
+	    M_WAITOK|M_ZERO);
-	memset((caddr_t)np->n_dirgens, 0, NFS_DIRHASHSIZ * sizeof (unsigned));
+	NFSDC_LOCK(np);
+	if (np->n_dirgens == NULL) {
+		np->n_dirgens = dirgens;
+		dirgens = NULL;
+	}
+	NFSDC_UNLOCK(np);
+	if (dirgens)
+		free(dirgens, M_NFSDIROFF);
 }
 
-static struct nfsdircache dzero = {0, 0, {0, 0}, {0, 0}, 0, 0, 0};
+static const struct nfsdircache dzero;
+
+static void nfs_unlinkdircache __P((struct nfsnode *np, struct nfsdircache *));
+static void nfs_putdircache_unlocked __P((struct nfsnode *,
+    struct nfsdircache *));
+
+static void
+nfs_unlinkdircache(np, ndp)
+	struct nfsnode *np;
+	struct nfsdircache *ndp;
+{
+
+	NFSDC_ASSERT_LOCKED(np);
+	KASSERT(ndp != &dzero);
+
+	if (LIST_NEXT(ndp, dc_hash) == (void *)-1)
+		return;
+
+	TAILQ_REMOVE(&np->n_dirchain, ndp, dc_chain);
+	LIST_REMOVE(ndp, dc_hash);
+	LIST_NEXT(ndp, dc_hash) = (void *)-1; /* mark as unlinked */
+
+	nfs_putdircache_unlocked(np, ndp);
+}
+
+void
+nfs_putdircache(np, ndp)
+	struct nfsnode *np;
+	struct nfsdircache *ndp;
+{
+	int ref;
+
+	if (ndp == &dzero)
+		return;
+
+	KASSERT(ndp->dc_refcnt > 0);
+	NFSDC_LOCK(np);
+	ref = --ndp->dc_refcnt;
+	NFSDC_UNLOCK(np);
+
+	if (ref == 0)
+		free(ndp, M_NFSDIROFF);
+}
+
+static void
+nfs_putdircache_unlocked(np, ndp)
+	struct nfsnode *np;
+	struct nfsdircache *ndp;
+{
+	int ref;
+
+	NFSDC_ASSERT_LOCKED(np);
+
+	if (ndp == &dzero)
+		return;
+
+	KASSERT(ndp->dc_refcnt > 0);
+	ref = --ndp->dc_refcnt;
+	if (ref == 0)
+		free(ndp, M_NFSDIROFF);
+}
 
 struct nfsdircache *
 nfs_searchdircache(vp, off, do32, hashent)
@@ -1261,7 +1341,8 @@ nfs_searchdircache(vp, off, do32, hashent)
 	 * Zero is always a valid cookie.
 	 */
 	if (off == 0)
-		return &dzero;
+		/* LINTED const cast away */
+		return (struct nfsdircache *)&dzero;
 
 	if (!np->n_dircache)
 		return NULL;
@@ -1281,6 +1362,8 @@ nfs_searchdircache(vp, off, do32, hashent)
 
 	if (hashent)
 		*hashent = (int)(ndhp - np->n_dircache);
+
+	NFSDC_LOCK(np);
 	if (do32) {
 		LIST_FOREACH(ndp, ndhp, dc_hash) {
 			if (ndp->dc_cookie32 == (u_int32_t)off) {
@@ -1289,10 +1372,11 @@ nfs_searchdircache(vp, off, do32, hashent)
 				 * start of a new block fetched from
 				 * the server.
 				 */
-				if (ndp->dc_blkno == -1) {
+				if (ndp->dc_flags & NFSDC_INVALID) {
 					ndp->dc_blkcookie = ndp->dc_cookie;
 					ndp->dc_blkno = np->n_dblkno++;
 					ndp->dc_entry = 0;
+					ndp->dc_flags &= ~NFSDC_INVALID;
 				}
 				break;
 			}
@@ -1303,6 +1387,9 @@ nfs_searchdircache(vp, off, do32, hashent)
 				break;
 		}
 	}
+	if (ndp != NULL)
+		ndp->dc_refcnt++;
+	NFSDC_UNLOCK(np);
 	return ndp;
 }
 
@@ -1316,10 +1403,20 @@ nfs_enterdircache(vp, off, blkoff, en, blkno)
 {
 	struct nfsnode *np = VTONFS(vp);
 	struct nfsdirhashhead *ndhp;
-	struct nfsdircache *ndp = NULL, *first;
+	struct nfsdircache *ndp = NULL;
+	struct nfsdircache *newndp = NULL;
 	struct nfsmount *nmp = VFSTONFS(vp->v_mount);
 	int hashent, gen, overwrite;
 
+	/*
+	 * XXX refuse entries for offset 0. amd(8) erroneously sets
+	 * cookie 0 for the '.' entry, making this necessary. This
+	 * isn't so bad, as 0 is a special case anyway.
+	 */
+	if (off == 0)
+		/* LINTED const cast away */
+		return (struct nfsdircache *)&dzero;
+
 	if (!np->n_dircache)
 		/*
 		 * XXX would like to do this in nfs_nget but vtype
@@ -1330,34 +1427,34 @@ nfs_enterdircache(vp, off, blkoff, en, blkno)
 	if ((nmp->nm_flag & NFSMNT_XLATECOOKIE) && !np->n_dirgens)
 		nfs_initdirxlatecookie(vp);
 
-	/*
+retry:
-	 * XXX refuse entries for offset 0. amd(8) erroneously sets
-	 * cookie 0 for the '.' entry, making this necessary. This
-	 * isn't so bad, as 0 is a special case anyway.
-	 */
-	if (off == 0)
-		return &dzero;
-
 	ndp = nfs_searchdircache(vp, off, 0, &hashent);
 
-	if (ndp && ndp->dc_blkno != -1) {
+	NFSDC_LOCK(np);
+	if (ndp && (ndp->dc_flags & NFSDC_INVALID) == 0) {
 		/*
 		 * Overwriting an old entry. Check if it's the same.
 		 * If so, just return. If not, remove the old entry.
 		 */
 		if (ndp->dc_blkcookie == blkoff && ndp->dc_entry == en)
-			return ndp;
+			goto done;
-		TAILQ_REMOVE(&np->n_dirchain, ndp, dc_chain);
+		nfs_unlinkdircache(np, ndp);
-		LIST_REMOVE(ndp, dc_hash);
+		nfs_putdircache_unlocked(np, ndp);
-		FREE(ndp, M_NFSDIROFF);
+		ndp = NULL;
-		ndp = 0;
 	}
 
 	ndhp = &np->n_dircache[hashent];
 
 	if (!ndp) {
-		MALLOC(ndp, struct nfsdircache *, sizeof (*ndp), M_NFSDIROFF,
+		if (newndp == NULL) {
-		    M_WAITOK);
+			NFSDC_UNLOCK(np);
+			newndp = malloc(sizeof(*ndp), M_NFSDIROFF, M_WAITOK);
+			newndp->dc_refcnt = 1;
+			LIST_NEXT(newndp, dc_hash) = (void *)-1;
+			goto retry;
+		}
+		ndp = newndp;
+		newndp = NULL;
 		overwrite = 0;
 		if (nmp->nm_flag & NFSMNT_XLATECOOKIE) {
 			/*
@@ -1388,7 +1485,7 @@ nfs_enterdircache(vp, off, blkoff, en, blkno)
 	ndp->dc_entry = en;
 
 	if (overwrite)
-		return ndp;
+		goto done;
 
 	/*
 	 * If the maximum directory cookie cache size has been reached
@@ -1398,15 +1495,19 @@ nfs_enterdircache(vp, off, blkoff, en, blkno)
 	 * loss.
 	 */
 	if (np->n_dircachesize == NFS_MAXDIRCACHE) {
-		first = TAILQ_FIRST(&np->n_dirchain);
+		nfs_unlinkdircache(np, TAILQ_FIRST(&np->n_dirchain));
-		TAILQ_REMOVE(&np->n_dirchain, first, dc_chain);
-		LIST_REMOVE(first, dc_hash);
-		FREE(first, M_NFSDIROFF);
 	} else
 		np->n_dircachesize++;
 		
+	KASSERT(ndp->dc_refcnt == 1);
 	LIST_INSERT_HEAD(ndhp, ndp, dc_hash);
 	TAILQ_INSERT_TAIL(&np->n_dirchain, ndp, dc_chain);
+	ndp->dc_refcnt++;
+done:
+	KASSERT(ndp->dc_refcnt > 0);
+	NFSDC_UNLOCK(np);
+	if (newndp)
+		nfs_putdircache(np, newndp);
 	return ndp;
 }
 
@@ -1427,11 +1528,11 @@ nfs_invaldircache(vp, forcefree)
 	if (!np->n_dircache)
 		return;
 
+	NFSDC_LOCK(np);
 	if (!(nmp->nm_flag & NFSMNT_XLATECOOKIE) || forcefree) {
-		while ((ndp = TAILQ_FIRST(&np->n_dirchain)) != 0) {
+		while ((ndp = TAILQ_FIRST(&np->n_dirchain)) != NULL) {
-			TAILQ_REMOVE(&np->n_dirchain, ndp, dc_chain);
+			KASSERT(!forcefree || ndp->dc_refcnt == 1);
-			LIST_REMOVE(ndp, dc_hash);
+			nfs_unlinkdircache(np, ndp);
-			FREE(ndp, M_NFSDIROFF);
 		}
 		np->n_dircachesize = 0;
 		if (forcefree && np->n_dirgens) {
@@ -1439,12 +1540,12 @@ nfs_invaldircache(vp, forcefree)
 			np->n_dirgens = NULL;
 		}
 	} else {
-		TAILQ_FOREACH(ndp, &np->n_dirchain, dc_chain) {
+		TAILQ_FOREACH(ndp, &np->n_dirchain, dc_chain)
-			ndp->dc_blkno = -1;
+			ndp->dc_flags |= NFSDC_INVALID;
-		}
 	}
 
 	np->n_dblkno = 1;
+	NFSDC_UNLOCK(np);
 }
 
 /*

sys/nfs/nfs_var.h

@@ -1,4 +1,4 @@
-/*	$NetBSD: nfs_var.h,v 1.45 2004/05/22 22:52:16 jonathan Exp $	*/
+/*	$NetBSD: nfs_var.h,v 1.46 2004/09/15 09:50:56 yamt Exp $	*/
 
 /*-
  * Copyright (c) 1996 The NetBSD Foundation, Inc.
@@ -261,6 +261,7 @@ void nfs_initdircache __P((struct vnode *));
 void nfs_initdirxlatecookie __P((struct vnode *));
 struct nfsdircache *nfs_searchdircache __P((struct vnode *, off_t, int, int *));
 struct nfsdircache *nfs_enterdircache __P((struct vnode *, off_t, off_t,						   int, daddr_t));
+void nfs_putdircache __P((struct nfsnode *, struct nfsdircache *));
 void nfs_invaldircache __P((struct vnode *, int));
 void nfs_init __P((void));
 int nfsm_loadattrcache __P((struct vnode **, struct mbuf **, caddr_t *,

sys/nfs/nfsnode.h

@@ -1,4 +1,4 @@
-/*	 $NetBSD: nfsnode.h,v 1.48 2004/08/24 20:09:44 yamt Exp $	*/
+/*	 $NetBSD: nfsnode.h,v 1.49 2004/09/15 09:50:56 yamt Exp $	*/
 
 /*
  * Copyright (c) 1989, 1993
@@ -80,11 +80,15 @@ struct nfsdircache {
 	off_t		dc_blkcookie;		/* Offset of block we're in */
 	LIST_ENTRY(nfsdircache) dc_hash;	/* Hash chain */
 	TAILQ_ENTRY(nfsdircache) dc_chain;	/* Least recently entered chn */
-	u_int32_t	dc_cookie32;		/* Key for 64<->32 xlate case */
 	daddr_t		dc_blkno;		/* Number of block we're in */
+	u_int32_t	dc_cookie32;		/* Key for 64<->32 xlate case */
 	int		dc_entry;		/* Entry number within block */
+	int		dc_refcnt;		/* Reference count */
+	int		dc_flags;		/* NFSDC_ flags */
 };
 
+#define	NFSDC_INVALID	1
+
 
 /*
  * The nfsnode is the nfs equivalent to ufs's inode. Any similarity