Re: [PATCH] udf: fix potential heap buffer overflow in handle_partition_descriptor().

[Jan Kara]

On Thu 09-04-26 02:06:45, Ashutosh Desai wrote:
> When the partition descriptor array needs to grow, the new allocation
> size is computed using the on-disk partitionNumber field rather than
> the number of entries that actually need to fit.
> 
> If a UDF image presents 33 unique partition descriptors and the 33rd
> has a partitionNumber in the range 1..32, ALIGN(partnum, 32) returns
> 32 - the same as the existing allocation size.  As a result the buffer
> does not grow, yet the new entry is written at index num_part_descs
> (now 32), one slot past the end of the 32-element array.  This ends up
> corrupting 8 bytes of adjacent heap memory with the volDescSeqNum and
> block number taken from the on-disk descriptor, both of which are
> under the control of whoever crafted the image.
> 
> On kernels where unprivileged users can mount filesystems (e.g. via
> user namespaces) this could be reached without any special privileges.
> 
> The straightforward fix is to base the new size on how many entries
> actually need to be stored (num_part_descs + 1) instead of relying on
> an untrusted on-disk value for that purpose.
> 
> Signed-off-by: Ashutosh Desai <ashutoshdesai993@xxxxxxxxx>

Thanks for the patch. Commit 08841b06fa6 ("udf: fix partition descriptor
append bookkeeping") queued in my tree should already fix this problem.

								Honza


> ---
>  fs/udf/super.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/udf/super.c b/fs/udf/super.c
> index 27f463fd1..9505b5c14 100644
> --- a/fs/udf/super.c
> +++ b/fs/udf/super.c
> @@ -1694,7 +1694,8 @@ static struct udf_vds_record *handle_partition_descriptor(
>  			return &(data->part_descs_loc[i].rec);
>  	if (data->num_part_descs >= data->size_part_descs) {
>  		struct part_desc_seq_scan_data *new_loc;
> -		unsigned int new_size = ALIGN(partnum, PART_DESC_ALLOC_STEP);
> +		unsigned int new_size = ALIGN(data->num_part_descs + 1,
> +					      PART_DESC_ALLOC_STEP);
>  
>  		new_loc = kzalloc_objs(*new_loc, new_size);
>  		if (!new_loc)
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR