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 17:13:35 +0200

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?
 
> diff --git a/sys/ufs/ffs/ffs_alloc.c b/sys/ufs/ffs/ffs_alloc.c
> index 14822193a03..9363637deee 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, map;
> +	u_int bit;
>  
>  	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;
> @@ -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