Fix races in msdosfs_lookup() and msdosfs_readdir(). These functions

can easily block in bread(), and then there was nothing to prevent the
static buffer (nambuf_{ptr,len,last_id}) being clobbered by another
thread.

The effects of the bug seem to have been limited to failed lookups and
mangled names in readdir(), since Giant locking provides enough
serialization to prevent concurrent calls to the functions that access
the buffer.  They were very obvious for multiple concurrent tree walks,
especially with a small cluster size.

The bug was introduced in msdosfs_conv.c 1.34 and associated changes,
and is in all releases starting with 5.2.

The fix is to allocate the buffer as a local variable and pass around
pointers to it like "_r" functions in libc do.  Stack use from this
is large but not too large.  This also fixes a memory leak on module
unload.

Reviewed by:	kib
Approved by:	re (kensmith)

sys/fs/msdosfs/direntry.h

@@ -133,21 +133,28 @@ struct winentry {
 #define DD_YEAR_SHIFT		9
 
 #ifdef _KERNEL
+struct mbnambuf {
+	size_t	nb_len;
+	int	nb_last_id;
+	char	nb_buf[WIN_MAXLEN + 1];
+};
+
 struct dirent;
 struct msdosfsmount;
 
-char	*mbnambuf_flush(struct dirent *dp);
-void	mbnambuf_init(void);
-void	mbnambuf_write(char *name, int id);
+char	*mbnambuf_flush(struct mbnambuf *nbp, struct dirent *dp);
+void	mbnambuf_init(struct mbnambuf *nbp);
+void	mbnambuf_write(struct mbnambuf *nbp, char *name, int id);
 int	dos2unixfn(u_char dn[11], u_char *un, int lower,
 	    struct msdosfsmount *pmp);
 int	unix2dosfn(const u_char *un, u_char dn[12], size_t unlen, u_int gen,
 	    struct msdosfsmount *pmp);
 int	unix2winfn(const u_char *un, size_t unlen, struct winentry *wep, int cnt,
 	    int chksum, struct msdosfsmount *pmp);
-int	winChkName(const u_char *un, size_t unlen, int chksum,
+int	winChkName(struct mbnambuf *nbp, const u_char *un, size_t unlen,
+	    int chksum, struct msdosfsmount *pmp);
+int	win2unixfn(struct mbnambuf *nbp, struct winentry *wep, int chksum,
 	    struct msdosfsmount *pmp);
-int	win2unixfn(struct winentry *wep, int chksum, struct msdosfsmount *pmp);
 u_int8_t winChksum(struct direntry *dep);
 int	winSlotCnt(const u_char *un, size_t unlen, struct msdosfsmount *pmp);
 size_t	winLenFixup(const u_char *un, size_t unlen);

sys/fs/msdosfs/msdosfs_conv.c

@@ -52,7 +52,6 @@
 #include <sys/systm.h>
 #include <sys/dirent.h>
 #include <sys/iconv.h>
-#include <sys/malloc.h>
 #include <sys/mount.h>
 
 #include <fs/msdosfs/bpb.h>
@@ -67,10 +66,6 @@ static u_int16_t unix2doschr(const u_char **, size_t *, struct msdosfsmount *);
 static u_int16_t win2unixchr(u_int16_t, struct msdosfsmount *);
 static u_int16_t unix2winchr(const u_char **, size_t *, int, struct msdosfsmount *);
 
-static char	*nambuf_ptr;
-static size_t	nambuf_len;
-static int	nambuf_last_id;
-
 /*
  * 0 - character disallowed in long file name.
  * 1 - character should be replaced by '_' in DOS file name, 
@@ -601,7 +596,8 @@ unix2winfn(un, unlen, wep, cnt, chksum, pmp)
  * Returns the checksum or -1 if no match
  */
 int
-winChkName(un, unlen, chksum, pmp)
+winChkName(nbp, un, unlen, chksum, pmp)
+	struct mbnambuf *nbp;
 	const u_char *un;
 	size_t unlen;
 	int chksum;
@@ -613,9 +609,9 @@ winChkName(un, unlen, chksum, pmp)
 	struct dirent dirbuf;
 
 	/*
-	 * We alread have winentry in mbnambuf
+	 * We already have winentry in *nbp.
 	 */
-	if (!mbnambuf_flush(&dirbuf) || !dirbuf.d_namlen)
+	if (!mbnambuf_flush(nbp, &dirbuf) || dirbuf.d_namlen == 0)
 		return -1;
 
 #ifdef MSDOSFS_DEBUG
@@ -650,7 +646,8 @@ winChkName(un, unlen, chksum, pmp)
  * Returns the checksum or -1 if impossible
  */
 int
-win2unixfn(wep, chksum, pmp)
+win2unixfn(nbp, wep, chksum, pmp)
+	struct mbnambuf *nbp;
 	struct winentry *wep;
 	int chksum;
 	struct msdosfsmount *pmp;
@@ -683,7 +680,7 @@ win2unixfn(wep, chksum, pmp)
 		switch (code) {
 		case 0:
 			*np = '\0';
-			mbnambuf_write(name, (wep->weCnt & WIN_CNT) - 1);
+			mbnambuf_write(nbp, name, (wep->weCnt & WIN_CNT) - 1);
 			return chksum;
 		case '/':
 			*np = '\0';
@@ -702,7 +699,7 @@ win2unixfn(wep, chksum, pmp)
 		switch (code) {
 		case 0:
 			*np = '\0';
-			mbnambuf_write(name, (wep->weCnt & WIN_CNT) - 1);
+			mbnambuf_write(nbp, name, (wep->weCnt & WIN_CNT) - 1);
 			return chksum;
 		case '/':
 			*np = '\0';
@@ -721,7 +718,7 @@ win2unixfn(wep, chksum, pmp)
 		switch (code) {
 		case 0:
 			*np = '\0';
-			mbnambuf_write(name, (wep->weCnt & WIN_CNT) - 1);
+			mbnambuf_write(nbp, name, (wep->weCnt & WIN_CNT) - 1);
 			return chksum;
 		case '/':
 			*np = '\0';
@@ -736,7 +733,7 @@ win2unixfn(wep, chksum, pmp)
 		cp += 2;
 	}
 	*np = '\0';
-	mbnambuf_write(name, (wep->weCnt & WIN_CNT) - 1);
+	mbnambuf_write(nbp, name, (wep->weCnt & WIN_CNT) - 1);
 	return chksum;
 }
 
@@ -1037,19 +1034,15 @@ unix2winchr(const u_char **instr, size_t *ilen, int lower, struct msdosfsmount *
 }
 
 /*
- * Initialize the temporary concatenation buffer (once) and mark it as
- * empty on subsequent calls.
+ * Initialize the temporary concatenation buffer.
  */
 void
-mbnambuf_init(void)
+mbnambuf_init(struct mbnambuf *nbp)
 {
 
-        if (nambuf_ptr == NULL) { 
-		nambuf_ptr = malloc(MAXNAMLEN + 1, M_MSDOSFSMNT, M_WAITOK);
-		nambuf_ptr[MAXNAMLEN] = '\0';
-	}
-	nambuf_len = 0;
-	nambuf_last_id = -1;
+	nbp->nb_len = 0;
+	nbp->nb_last_id = -1;
+	nbp->nb_buf[sizeof(nbp->nb_buf) - 1] = '\0';
 }
 
 /*
@@ -1062,30 +1055,31 @@ mbnambuf_init(void)
  * WIN_CHARS bytes when they are first encountered.
  */
 void
-mbnambuf_write(char *name, int id)
+mbnambuf_write(struct mbnambuf *nbp, char *name, int id)
 {
-	size_t count;
 	char *slot;
+	size_t count, newlen;
 
-	KASSERT(nambuf_len == 0 || id == nambuf_last_id - 1,
-	    ("non-decreasing id, id %d last id %d", id, nambuf_last_id));
+	KASSERT(nbp->nb_len == 0 || id == nbp->nb_last_id - 1,
+	    ("non-decreasing id: id %d, last id %d", id, nbp->nb_last_id));
 
-	/* Store this substring in a WIN_CHAR-aligned slot. */
-	slot = nambuf_ptr + (id * WIN_CHARS);
+	/* Will store this substring in a WIN_CHARS-aligned slot. */
+	slot = &nbp->nb_buf[id * WIN_CHARS];
 	count = strlen(name);
-	if (nambuf_len + count > MAXNAMLEN) {
-		printf("msdosfs: file name %zu too long\n", nambuf_len + count);
+	newlen = nbp->nb_len + count;
+	if (newlen > WIN_MAXLEN || newlen > MAXNAMLEN) {
+		printf("msdosfs: file name length %zu too large\n", newlen);
 		return;
 	}
 
 	/* Shift suffix upwards by the amount length exceeds WIN_CHARS. */
-	if (count > WIN_CHARS && nambuf_len != 0)
-		bcopy(slot + WIN_CHARS, slot + count, nambuf_len);
+	if (count > WIN_CHARS && nbp->nb_len != 0)
+		bcopy(slot + WIN_CHARS, slot + count, nbp->nb_len);
 
 	/* Copy in the substring to its slot and update length so far. */
 	bcopy(name, slot, count);
-	nambuf_len += count;
-	nambuf_last_id = id;
+	nbp->nb_len = newlen;
+	nbp->nb_last_id = id;
 }
 
 /*
@@ -1096,17 +1090,17 @@ mbnambuf_write(char *name, int id)
  * have been written via mbnambuf_write(), the result will be incorrect.
  */
 char *
-mbnambuf_flush(struct dirent *dp)
+mbnambuf_flush(struct mbnambuf *nbp, struct dirent *dp)
 {
 
-	if (nambuf_len > sizeof(dp->d_name) - 1) {
-		mbnambuf_init();
+	if (nbp->nb_len > sizeof(dp->d_name) - 1) {
+		mbnambuf_init(nbp);
 		return (NULL);
 	}
-	bcopy(nambuf_ptr, dp->d_name, nambuf_len);
-	dp->d_name[nambuf_len] = '\0';
-	dp->d_namlen = nambuf_len;
+	bcopy(&nbp->nb_buf[0], dp->d_name, nbp->nb_len);
+	dp->d_name[nbp->nb_len] = '\0';
+	dp->d_namlen = nbp->nb_len;
 
-	mbnambuf_init();
+	mbnambuf_init(nbp);
 	return (dp->d_name);
 }

sys/fs/msdosfs/msdosfs_lookup.c

@@ -84,6 +84,7 @@ msdosfs_lookup(ap)
 		struct componentname *a_cnp;
 	} */ *ap;
 {
+	struct mbnambuf nb;
 	struct vnode *vdp = ap->a_dvp;
 	struct vnode **vpp = ap->a_vpp;
 	struct componentname *cnp = ap->a_cnp;
@@ -185,7 +186,7 @@ msdosfs_lookup(ap)
 	 * by cnp->cn_nameptr.
 	 */
 	tdp = NULL;
-	mbnambuf_init();
+	mbnambuf_init(&nb);
 	/*
 	 * The outer loop ranges over the clusters that make up the
 	 * directory.  Note that the root directory is different from all
@@ -225,7 +226,7 @@ msdosfs_lookup(ap)
 				 * Drop memory of previous long matches
 				 */
 				chksum = -1;
-				mbnambuf_init();
+				mbnambuf_init(&nb);
 
 				if (slotcount < wincnt) {
 					slotcount++;
@@ -250,16 +251,15 @@ msdosfs_lookup(ap)
 					if (pmp->pm_flags & MSDOSFSMNT_SHORTNAME)
 						continue;
 
-					chksum = win2unixfn((struct winentry *)dep,
-							    chksum,
-							    pmp);
+					chksum = win2unixfn(&nb,
+					    (struct winentry *)dep, chksum,
+					    pmp);
 					continue;
 				}
 
-				chksum = winChkName((const u_char *)cnp->cn_nameptr,
-						    unlen,
-						    chksum,
-						    pmp);
+				chksum = winChkName(&nb,
+				    (const u_char *)cnp->cn_nameptr, unlen,
+				    chksum, pmp);
 				if (chksum == -2) {
 					chksum = -1;
 					continue;

sys/fs/msdosfs/msdosfs_vnops.c

@@ -1510,6 +1510,7 @@ msdosfs_readdir(ap)
 		u_long **a_cookies;
 	} */ *ap;
 {
+	struct mbnambuf nb;
 	int error = 0;
 	int diff;
 	long n;
@@ -1629,7 +1630,7 @@ msdosfs_readdir(ap)
 		}
 	}
 
-	mbnambuf_init();
+	mbnambuf_init(&nb);
 	off = offset;
 	while (uio->uio_resid > 0) {
 		lbn = de_cluster(pmp, offset - bias);
@@ -1676,7 +1677,7 @@ msdosfs_readdir(ap)
 			 */
 			if (dentp->deName[0] == SLOT_DELETED) {
 				chksum = -1;
-				mbnambuf_init();
+				mbnambuf_init(&nb);
 				continue;
 			}
 
@@ -1686,8 +1687,8 @@ msdosfs_readdir(ap)
 			if (dentp->deAttributes == ATTR_WIN95) {
 				if (pmp->pm_flags & MSDOSFSMNT_SHORTNAME)
 					continue;
-				chksum = win2unixfn((struct winentry *)dentp,
-					chksum, pmp);
+				chksum = win2unixfn(&nb,
+				    (struct winentry *)dentp, chksum, pmp);
 				continue;
 			}
 
@@ -1696,7 +1697,7 @@ msdosfs_readdir(ap)
 			 */
 			if (dentp->deAttributes & ATTR_VOLUME) {
 				chksum = -1;
-				mbnambuf_init();
+				mbnambuf_init(&nb);
 				continue;
 			}
 			/*
@@ -1738,9 +1739,9 @@ msdosfs_readdir(ap)
 					((pmp->pm_flags & MSDOSFSMNT_SHORTNAME) ?
 					(LCASE_BASE | LCASE_EXT) : 0),
 				    pmp);
-				mbnambuf_init();
+				mbnambuf_init(&nb);
 			} else
-				mbnambuf_flush(&dirbuf);
+				mbnambuf_flush(&nb, &dirbuf);
 			chksum = -1;
 			dirbuf.d_reclen = GENERIC_DIRSIZ(&dirbuf);
 			if (uio->uio_resid < dirbuf.d_reclen) {