From: Rafael Sadowski <rafael@sizeofvoid.org>
Subject: Re: Avoid Undefined Behavior in ffs_clusteracct()
To: tech@openbsd.org
Date: Sat, 7 Jun 2025 12:19:02 +0200

On Wed Jun 04, 2025 at 08:29:46PM +0200, Claudio Jeker wrote:
> On Wed, Jun 04, 2025 at 05:44:04PM +0200, Rafael Sadowski wrote:
> > 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.
>  
> Actually the NetBSD commit does not fix anything.
> The problem is not bit or map. All shift operations are limited to be
> between 0 and 7 apart from one issue. start can be -1 if blockno is 0.
> 
> This was fixed properly in a later commit I think.
>  

Thank you, I can see that now too.

When blkno is 0, start = -1, causing the backward cluster scan to
access memory before the freemapp array via mapp--.

Fix by checking if start < 0 before the backward scan. When blkno is 0,
there are no blocks to scan backward from, so back = 0.

diff --git a/sys/ufs/ffs/ffs_alloc.c b/sys/ufs/ffs/ffs_alloc.c
index 14822193a03..be205841ec9 100644
--- a/sys/ufs/ffs/ffs_alloc.c
+++ b/sys/ufs/ffs/ffs_alloc.c
@@ -1519,20 +1519,24 @@ ffs_clusteracct(struct fs *fs, struct cg *cgp, daddr_t blkno, int cnt)
 	end = start - fs->fs_contigsumsize;
 	if (end < 0)
 		end = -1;
-	mapp = &freemapp[start / NBBY];
-	map = *mapp--;
-	bit = 1 << (start % NBBY);
-	for (i = start; i > end; i--) {
-		if ((map & bit) == 0)
-			break;
-		if ((i & (NBBY - 1)) != 0) {
-			bit >>= 1;
-		} else {
-			map = *mapp--;
-			bit = 1 << (NBBY - 1);
+	if (start < 0) {
+		back = 0;
+	} else {
+		mapp = &freemapp[start / NBBY];
+		map = *mapp--;
+		bit = 1 << (start % NBBY);
+		for (i = start; i > end; i--) {
+			if ((map & bit) == 0)
+				break;
+			if ((i & (NBBY - 1)) != 0) {
+				bit >>= 1;
+			} else {
+				map = *mapp--;
+				bit = 1 << (NBBY - 1);
+			}
 		}
+		back = start - i;
 	}
-	back = start - i;
 	/*
 	 * Account for old cluster and the possibly new forward and
 	 * back clusters.