ufs: Rework shortlink handling to avoid subobject overflows

Shortlinks occupy the space of both di_db and di_ib when used. However, everywhere that wants to read or write a shortlink takes a pointer do di_db and promptly runs off the end of it into di_ib. This is fine on most architectures, if a little dodgy. However, on CHERI, the compiler can optionally restrict the bounds on pointers to subobjects to just that subobject, in order to mitigate intra-object buffer overflows, and this is enabled in CheriBSD's pure-capability kernels. Instead, clean this up by inserting a union such that a new di_shortlink can be added with the right size and element type, avoiding the need to cast and allowing the use of the DIP macro to access the field. This also mirrors how the ext2fs code implements extents support, with the exact same structure other than having a uint32_t i_data[] instead of a char di_shortlink[]. Reviewed by: mckusick, jhb Differential Revision: https://reviews.freebsd.org/D33650
T-bagwell · Jan 2, 2022 · 5b13fa7 · 5b13fa7
1 parent 04fd468
commit 5b13fa7
Show file tree

Hide file tree

Showing 10 changed files with 35 additions and 31 deletions.
diff --git a/sbin/dump/traverse.c b/sbin/dump/traverse.c
@@ -525,12 +525,8 @@ dumpino(union dinode *dp, ino_t ino)
 			spcl.c_count = 1;
 			added = appendextdata(dp);
 			writeheader(ino);
-			if (sblock->fs_magic == FS_UFS1_MAGIC)
-				memmove(buf, (caddr_t)dp->dp1.di_db,
-				    (u_long)DIP(dp, di_size));
-			else
-				memmove(buf, (caddr_t)dp->dp2.di_db,
-				    (u_long)DIP(dp, di_size));
+			memmove(buf, DIP(dp, di_shortlink),
+			    (u_long)DIP(dp, di_size));
 			buf[DIP(dp, di_size)] = '\0';
 			writerec(buf, 0);
 			writeextdata(dp, ino, added);

diff --git a/sbin/fsdb/fsdbutil.c b/sbin/fsdb/fsdbutil.c
@@ -136,11 +136,8 @@ printstat(const char *cp, ino_t inum, union dinode *dp)
 	if (DIP(dp, di_size) > 0 &&
 	    DIP(dp, di_size) < sblock.fs_maxsymlinklen &&
 	    DIP(dp, di_blocks) == 0) {
-	    if (sblock.fs_magic == FS_UFS1_MAGIC)
-		p = (caddr_t)dp->dp1.di_db;
-	    else
-		p = (caddr_t)dp->dp2.di_db;
-	    printf(" to `%.*s'\n", (int) DIP(dp, di_size), p);
+	    printf(" to `%.*s'\n", (int) DIP(dp, di_size),
+		DIP(dp, di_shortlink));
 	} else {
 	    putchar('\n');
 	}

diff --git a/stand/libsa/ufs.c b/stand/libsa/ufs.c
@@ -638,11 +638,8 @@ ufs_open(const char *upath, struct open_file *f)
 			bcopy(cp, &namebuf[link_len], len + 1);
 
 			if (link_len < fs->fs_maxsymlinklen) {
-				if (fp->f_fs->fs_magic == FS_UFS1_MAGIC)
-					cp = (caddr_t)(fp->f_di.di1.di_db);
-				else
-					cp = (caddr_t)(fp->f_di.di2.di_db);
-				bcopy(cp, namebuf, (unsigned) link_len);
+				bcopy(DIP(fp, di_shortlink), namebuf,
+				    (unsigned) link_len);
 			} else {
 				/*
 				 * Read file for symbolic link

diff --git a/sys/ufs/ffs/ffs_inode.c b/sys/ufs/ffs/ffs_inode.c
@@ -342,7 +342,7 @@ ffs_truncate(vp, length, flags, cred)
 		if (length != 0)
 			panic("ffs_truncate: partial truncate of symlink");
 #endif
-		bzero(SHORTLINK(ip), (u_int)ip->i_size);
+		bzero(DIP(ip, i_shortlink), (u_int)ip->i_size);
 		ip->i_size = 0;
 		DIP_SET(ip, i_size, 0);
 		UFS_INODE_SET_FLAG(ip, IN_SIZEMOD | IN_CHANGE | IN_UPDATE);

diff --git a/sys/ufs/ufs/dinode.h b/sys/ufs/ufs/dinode.h
@@ -145,8 +145,16 @@ struct ufs2_dinode {
 	u_int32_t	di_flags;	/*  88: Status flags (chflags). */
 	u_int32_t	di_extsize;	/*  92: External attributes size. */
 	ufs2_daddr_t	di_extb[UFS_NXADDR];/* 96: External attributes block. */
-	ufs2_daddr_t	di_db[UFS_NDADDR]; /* 112: Direct disk blocks. */
-	ufs2_daddr_t	di_ib[UFS_NIADDR]; /* 208: Indirect disk blocks. */
+	union {
+		struct {
+			ufs2_daddr_t	di_db /* 112: Direct disk blocks. */
+			    [UFS_NDADDR];
+			ufs2_daddr_t	di_ib /* 208: Indirect disk blocks. */
+			    [UFS_NIADDR];
+		};
+		char	di_shortlink	/* 112: Embedded symbolic link. */
+		    [(UFS_NDADDR + UFS_NIADDR) * sizeof(ufs2_daddr_t)];
+	};
 	u_int64_t	di_modrev;	/* 232: i_modrev for NFSv4 */
 	uint32_t	di_freelink;	/* 240: SUJ: Next unlinked inode. */
 	uint32_t	di_ckhash;	/* 244: if CK_INODE, its check-hash */
@@ -179,8 +187,16 @@ struct ufs1_dinode {
 	int32_t		di_mtimensec;	/*  28: Last modified time. */
 	int32_t		di_ctime;	/*  32: Last inode change time. */
 	int32_t		di_ctimensec;	/*  36: Last inode change time. */
-	ufs1_daddr_t	di_db[UFS_NDADDR]; /*  40: Direct disk blocks. */
-	ufs1_daddr_t	di_ib[UFS_NIADDR]; /*  88: Indirect disk blocks. */
+	union {
+		struct {
+			ufs1_daddr_t	di_db /*  40: Direct disk blocks. */
+			    [UFS_NDADDR];
+			ufs1_daddr_t	di_ib /*  88: Indirect disk blocks. */
+			    [UFS_NIADDR];
+		};
+		char	di_shortlink	/*  40: Embedded symbolic link. */
+		    [(UFS_NDADDR + UFS_NIADDR) * sizeof(ufs1_daddr_t)];
+	};
 	u_int32_t	di_flags;	/* 100: Status flags (chflags). */
 	u_int32_t	di_blocks;	/* 104: Blocks actually held. */
 	u_int32_t	di_gen;		/* 108: Generation number. */

diff --git a/sys/ufs/ufs/inode.h b/sys/ufs/ufs/inode.h
@@ -246,8 +246,6 @@ I_IS_UFS2(const struct inode *ip)
 		(ip)->i_din2->d##field = (val); 		\
 	} while (0)
 
-#define	SHORTLINK(ip)	(I_IS_UFS1(ip) ?			\
-    (caddr_t)(ip)->i_din1->di_db : (caddr_t)(ip)->i_din2->di_db)
 #define	IS_SNAPSHOT(ip)		((ip)->i_flags & SF_SNAPSHOT)
 
 /*

diff --git a/sys/ufs/ufs/ufs_vnops.c b/sys/ufs/ufs/ufs_vnops.c
@@ -2311,7 +2311,7 @@ ufs_symlink(ap)
 	len = strlen(ap->a_target);
 	if (len < VFSTOUFS(vp->v_mount)->um_maxsymlinklen) {
 		ip = VTOI(vp);
-		bcopy(ap->a_target, SHORTLINK(ip), len);
+		bcopy(ap->a_target, DIP(ip, i_shortlink), len);
 		ip->i_size = len;
 		DIP_SET(ip, i_size, len);
 		UFS_INODE_SET_FLAG(ip, IN_SIZEMOD | IN_CHANGE | IN_UPDATE);
@@ -2481,7 +2481,7 @@ ufs_readlink(ap)
 
 	isize = ip->i_size;
 	if (isize < VFSTOUFS(vp->v_mount)->um_maxsymlinklen)
-		return (uiomove(SHORTLINK(ip), isize, ap->a_uio));
+		return (uiomove(DIP(ip, i_shortlink), isize, ap->a_uio));
 	return (VOP_READ(vp, ap->a_uio, 0, ap->a_cred));
 }
 

diff --git a/tools/diag/prtblknos/prtblknos.c b/tools/diag/prtblknos/prtblknos.c
@@ -99,8 +99,8 @@ prtblknos(fs, dp)
 		if (size < fs->fs_maxsymlinklen) {
 			printf("symbolic link referencing %s\n",
 			    (fs->fs_magic == FS_UFS1_MAGIC) ?
-			    (char *)dp->dp1.di_db :
-			    (char *)dp->dp2.di_db);
+			    dp->dp1.di_shortlink :
+			    dp->dp2.di_shortlink);
 			return;
 		}
 		printf("symbolic link\n");

diff --git a/usr.sbin/makefs/ffs.c b/usr.sbin/makefs/ffs.c
@@ -704,7 +704,7 @@ ffs_build_dinode1(struct ufs1_dinode *dinp, dirbuf_t *dbufp, fsnode *cur,
 	} else if (S_ISLNK(cur->type)) {	/* symlink */
 		slen = strlen(cur->symlink);
 		if (slen < UFS1_MAXSYMLINKLEN) {	/* short link */
-			memcpy(dinp->di_db, cur->symlink, slen);
+			memcpy(dinp->di_shortlink, cur->symlink, slen);
 		} else
 			membuf = cur->symlink;
 		dinp->di_size = slen;
@@ -763,7 +763,7 @@ ffs_build_dinode2(struct ufs2_dinode *dinp, dirbuf_t *dbufp, fsnode *cur,
 	} else if (S_ISLNK(cur->type)) {	/* symlink */
 		slen = strlen(cur->symlink);
 		if (slen < UFS2_MAXSYMLINKLEN) {	/* short link */
-			memcpy(dinp->di_db, cur->symlink, slen);
+			memcpy(dinp->di_shortlink, cur->symlink, slen);
 		} else
 			membuf = cur->symlink;
 		dinp->di_size = slen;

diff --git a/usr.sbin/makefs/ffs/ufs_inode.h b/usr.sbin/makefs/ffs/ufs_inode.h
@@ -68,7 +68,7 @@ struct inode {
 #define	i_ffs1_mtimensec	i_din.ffs1_din.di_mtimensec
 #define	i_ffs1_nlink		i_din.ffs1_din.di_nlink
 #define	i_ffs1_rdev		i_din.ffs1_din.di_rdev
-#define	i_ffs1_shortlink	i_din.ffs1_din.db
+#define	i_ffs1_shortlink	i_din.ffs1_din.di_shortlink
 #define	i_ffs1_size		i_din.ffs1_din.di_size
 #define	i_ffs1_uid		i_din.ffs1_din.di_uid
 
@@ -89,7 +89,7 @@ struct inode {
 #define	i_ffs2_mtimensec	i_din.ffs2_din.di_mtimensec
 #define	i_ffs2_nlink		i_din.ffs2_din.di_nlink
 #define	i_ffs2_rdev		i_din.ffs2_din.di_rdev
-#define	i_ffs2_shortlink	i_din.ffs2_din.db
+#define	i_ffs2_shortlink	i_din.ffs2_din.di_shortlink
 #define	i_ffs2_size		i_din.ffs2_din.di_size
 #define	i_ffs2_uid		i_din.ffs2_din.di_uid