Re: [PATCH] isofs: fix Rock Ridge CE extent validation on multisession media

[Jan Kara]

On Mon 07-09-26 16:26:02, Baolin Liu wrote:
> From: Baolin Liu <liubaolin@xxxxxxxxxx>
> 
> Commit a36d990f5913 ("isofs: validate Rock Ridge CE continuation extent
> against volume size") compares the CE extent directly with s_nzones.  The
> extent is an absolute block number, while s_nzones is the number of blocks
> relative to the selected ISO session.  The comparison is therefore wrong
> when a multisession disc starts at a non-zero block.
> 
> isofs_get_last_session() selects the last session on multisession
> media, and its volume descriptors describe a volume beginning at that
> session's LBA.
> For example, a session beginning at LBA 45447 with 64 blocks can contain a
> valid CE at absolute LBA 45467.  The existing check rejects that CE, so the
> ER continuation record is not read, Rock Ridge is disabled, and the mount
> falls back to Joliet names.
> 
> Save the selected session start in filesystem-block units and validate the
> CE extent against the half-open interval [session_start, session_end).
> Scale the session length to the same block units and retain a separate
> block-device limit.  The lower bound is intentional: accepting arbitrary
> blocks before the selected session could make a CE read data from a
> previous session or another filesystem on the device.
> 
> Only apply the bounds check when cont_extent is non-zero.  A zero extent is
> the in-memory sentinel indicating that no CE continuation was found, rather
> than a request to read block zero.
> 
> For a single-session image, session_start is zero and the effective volume
> boundary remains unchanged.
> 
> Build-tested with:
> 
>     make CONFIG_RUST= CONFIG_RUST_DRIVERS= fs/isofs/
> 
> Fixes: a36d990f5913 ("isofs: validate Rock Ridge CE continuation extent against volume size")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Baolin Liu <liubaolin@xxxxxxxxxx>

Thanks for the fix! I've taken the patch to my tree. I've slighly cleaned
up the types (use sector_t instead of u64). I've also remove the check
against sb_bdev_nr_blocks() - if anything, we should have check s_nzones
fits within the block device earlier, not in Rockridge parsing code.

								Honza

> @@ -101,11 +107,14 @@ static int rock_continue(struct rock_state *rs)
>  		goto out;
>  	}
>  
> -	if ((unsigned)rs->cont_extent >= ISOFS_SB(rs->inode->i_sb)->s_nzones) {
> +	if (rs->cont_extent &&
> +	    (extent < sbi->s_session_start ||
> +	     extent >= session_end ||
> +	     extent >= sb_bdev_nr_blocks(sb))) {
>  		printk(KERN_NOTICE "rock: corrupted directory entry. "
>  			"extent=%u out of volume (nzones=%lu)\n",
>  			(unsigned)rs->cont_extent,
> -			ISOFS_SB(rs->inode->i_sb)->s_nzones);
> +			sbi->s_nzones);
>  		ret = -EIO;
>  		goto out;
>  	}
> -- 
> 2.51.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR