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@gmail.com>
---
 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

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@gmail.com>

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@suse.com>
SUSE Labs, CR