Re: [PATCH 2/2] ufs: validate cylinder group metadata before caching it

On Sat 01-08-26 10:12:58, Ali Ahmet Memis wrote:
> ufs_read_cylinder() copies the cylinder group index and the rotor
> positions straight from the on-disk group and caches them without any
> check:
> 
> 	ucpi->c_cgx    = fs32_to_cpu(sb, ucg->cg_cgx);
> 	ucpi->c_rotor  = fs32_to_cpu(sb, ucg->cg_rotor);
> 	ucpi->c_frotor = fs32_to_cpu(sb, ucg->cg_frotor);
> 	ucpi->c_irotor = fs32_to_cpu(sb, ucg->cg_irotor);
> 
> They are then used as indices during allocation and free:
> 
>   - c_cgx indexes the cylinder summary array as
>     UFS_SB(sb)->fs_cs(ucpi->c_cgx), so a value past s_ncg writes a 32
>     bit count outside the s_csp allocation.
> 
>   - c_frotor becomes a bitmap scan start, start = c_frotor >> 3, and
>     then length = ((s_fpg + 7) >> 3) - start. A start beyond the block
>     bitmap wraps the unsigned length to a huge value, so ubh_scanc()
>     walks far past the cylinder group buffers. c_irotor drives the
>     inode bitmap the same way.
> 
> A crafted image can set any of these freely, turning an ordinary
> allocation into an out of bounds access.
> 
> Reject a cylinder group whose recorded index does not match the group
> being read, or whose rotors fall outside the group, before the metadata
> is cached. Valid filesystems keep cg_cgx equal to the group number and
> the rotors within the group, so only malformed images are rejected.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: [email protected]
> Signed-off-by: Ali Ahmet Memis <[email protected]>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

								Honza

> ---
>  fs/ufs/cylinder.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/fs/ufs/cylinder.c b/fs/ufs/cylinder.c
> index a2813270c..b930ee1cf 100644
> --- a/fs/ufs/cylinder.c
> +++ b/fs/ufs/cylinder.c
> @@ -68,6 +68,16 @@ static bool ufs_read_cylinder(struct super_block *sb,
>  	ucpi->c_clustersumoff = fs32_to_cpu(sb, ucg->cg_u.cg_44.cg_clustersumoff);
>  	ucpi->c_clusteroff = fs32_to_cpu(sb, ucg->cg_u.cg_44.cg_clusteroff);
>  	ucpi->c_nclusterblks = fs32_to_cpu(sb, ucg->cg_u.cg_44.cg_nclusterblks);
> +
> +	/* these on-disk values become array and bitmap indices */
> +	if (ucpi->c_cgx != cgno ||
> +	    ucpi->c_rotor >= uspi->s_fpg ||
> +	    ucpi->c_frotor >= uspi->s_fpg ||
> +	    ucpi->c_irotor >= uspi->s_ipg) {
> +		ufs_error(sb, __func__,
> +			  "inconsistent metadata in cylinder group %u\n", cgno);
> +		goto failed;
> +	}
>  	UFSD("EXIT\n");
>  	return true;
>  	
> -- 
> 2.54.0
> 
-- 
Jan Kara <[email protected]>
SUSE Labs, CR