Re: [PATCH] udf: validate partition reference before freeing blocks

[Jan Kara]

On Thu 11-06-26 14:36:35, Kyle Zeng wrote:
> UDF long allocation descriptors can carry an on-disk
> partitionReferenceNum. The truncate/free path passes that value to
> udf_free_blocks(), which indexes s_partmaps[partition] before checking
> that partition is smaller than s_partitions.
> 
> A crafted writable image with one partition and a long allocation
> descriptor that references partition 1 can therefore make
> udf_free_blocks() read past the allocated partition map array when
> truncating the file.
> 
> Validate the partition reference before forming the map pointer. Also
> compare the checked end block, including the caller-supplied offset,
> against the partition length; the previous test computed blk but then
> compared logicalBlockNum + count.
> 
> Assisted-by: Codex:gpt-5.5
> Signed-off-by: Kyle Zeng <kylebot@xxxxxxxxxx>

The patch looks sane but it would make more sense to check the returned
extent has valid partition already in udf_current_aext(). That would be
much more robust solution, possibly fixing more sites potentially dealin
with invalid extents.

								Honza

> ---
>  fs/udf/balloc.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/udf/balloc.c b/fs/udf/balloc.c
> index 807c493..9d5fe22 100644
> --- a/fs/udf/balloc.c
> +++ b/fs/udf/balloc.c
> @@ -656,13 +656,21 @@ void udf_free_blocks(struct super_block *sb, struct inode *inode,
>  		     struct kernel_lb_addr *bloc, uint32_t offset,
>  		     uint32_t count)
>  {
> +	struct udf_sb_info *sbi = UDF_SB(sb);
>  	uint16_t partition = bloc->partitionReferenceNum;
> -	struct udf_part_map *map = &UDF_SB(sb)->s_partmaps[partition];
> +	struct udf_part_map *map;
>  	uint32_t blk;
>  
> +	if (partition >= sbi->s_partitions) {
> +		udf_debug("Invalid partition reference %u (partitions %u)\n",
> +			  partition, sbi->s_partitions);
> +		return;
> +	}
> +
> +	map = &sbi->s_partmaps[partition];
>  	if (check_add_overflow(bloc->logicalBlockNum, offset, &blk) ||
>  	    check_add_overflow(blk, count, &blk) ||
> -	    bloc->logicalBlockNum + count > map->s_partition_len) {
> +	    blk > map->s_partition_len) {
>  		udf_debug("Invalid request to free blocks: (%d, %u), off %u, "
>  			  "len %u, partition len %u\n",
>  			  partition, bloc->logicalBlockNum, offset, count,
> -- 
> 2.54.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR