From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: Avoid Undefined Behavior in ffs_clusteracct()
To: Rafael Sadowski <rafael@sizeofvoid.org>
Cc: tech@openbsd.org
Date: Wed, 4 Jun 2025 20:29:46 +0200

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.
 
> 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;
> 

-- 
:wq Claudio