From: Rafael Sadowski <rafael@sizeofvoid.org>
Subject: Re: Avoid Undefined Behavior in ffs_clusteracct()
To: tech@openbsd.org
Date: Wed, 4 Jun 2025 17:44:04 +0200

On Wed Jun 04, 2025 at 05:13:35PM +0200, Claudio Jeker wrote:
> On Wed, Jun 04, 2025 at 04:41:34PM +0200, Rafael Sadowski wrote:
> > Backport commit from NetBSD with commit message:
> > 
> > Avoid Undefined Behavior in ffs_clusteracct()
> > 
> > Change the type of 'bit' variable from int to unsigned int and use unsigned
> > values consistently.
> > 
> > sys/ufs/ffs/ffs_subr.c:336:10, shift exponent -1 is negative
> > 
> > Detected with Kernel Undefined Behavior Sanitizer.
> > 
> > Tested on amd64 over days. OK?
> 
> Shouldn't map be u_int as well?

Yes. Since u_char is unsigned, map should also be unsigned. I also
replaced "1" with "1U" to be consistent.


diff --git a/sys/ufs/ffs/ffs_alloc.c b/sys/ufs/ffs/ffs_alloc.c
index 14822193a03..fc2b4dc130d 100644
--- a/sys/ufs/ffs/ffs_alloc.c
+++ b/sys/ufs/ffs/ffs_alloc.c
@@ -1478,7 +1478,8 @@ ffs_clusteracct(struct fs *fs, struct cg *cgp, daddr_t blkno, int cnt)
 	int32_t *sump;
 	int32_t *lp;
 	u_char *freemapp, *mapp;
-	int i, start, end, forw, back, map, bit;
+	int i, start, end, forw, back;
+	u_int bit, map;
 
 	if (fs->fs_contigsumsize <= 0)
 		return;
@@ -1500,7 +1501,7 @@ ffs_clusteracct(struct fs *fs, struct cg *cgp, daddr_t blkno, int cnt)
 		end = cgp->cg_nclusterblks;
 	mapp = &freemapp[start / NBBY];
 	map = *mapp++;
-	bit = 1 << (start % NBBY);
+	bit = 1U << (start % NBBY);
 	for (i = start; i < end; i++) {
 		if ((map & bit) == 0)
 			break;
@@ -1508,7 +1509,7 @@ ffs_clusteracct(struct fs *fs, struct cg *cgp, daddr_t blkno, int cnt)
 			bit <<= 1;
 		} else {
 			map = *mapp++;
-			bit = 1;
+			bit = 1U;
 		}
 	}
 	forw = i - start;
@@ -1521,7 +1522,7 @@ ffs_clusteracct(struct fs *fs, struct cg *cgp, daddr_t blkno, int cnt)
 		end = -1;
 	mapp = &freemapp[start / NBBY];
 	map = *mapp--;
-	bit = 1 << (start % NBBY);
+	bit = 1U << (start % NBBY);
 	for (i = start; i > end; i--) {
 		if ((map & bit) == 0)
 			break;
@@ -1529,7 +1530,7 @@ ffs_clusteracct(struct fs *fs, struct cg *cgp, daddr_t blkno, int cnt)
 			bit >>= 1;
 		} else {
 			map = *mapp--;
-			bit = 1 << (NBBY - 1);
+			bit = 1U << (NBBY - 1);
 		}
 	}
 	back = start - i;