Re: [PATCH v3] ext4: don't read past the UAPI struct in GROUP_ADD

[Jan Kara]

On Mon 19-10-26 21:16:55, Kaixuan Li wrote:
> EXT4_IOC_GROUP_ADD casts the user pointer to struct ext4_new_group_input,
> its UAPI type, but sizes the copy by struct ext4_new_group_data, the
> internal type:
> 
> 	struct ext4_new_group_data input;
> 
> 	if (copy_from_user(&input, (struct ext4_new_group_input __user *)arg,
> 			sizeof(input)))
> 
> The UAPI struct is 40 bytes, the internal one 48.  A caller that follows
> the UAPI and allocates 40 bytes gets 8 bytes read past its object, and the
> ioctl returns EFAULT when those bytes are unmapped.  The two structs have
> differed in size for as long as I can see back -- v6.12.9, v7.2.4 and
> current master all have 40 against 48 -- so the copy has been over-reading
> the UAPI object throughout.
> 
> The first 40 bytes of the two structs have the same layout, with the UAPI
> 'unused' in the slot of mdata_blocks, so copying that much fills every
> field the UAPI defines.  free_clusters_count, the one field past it, is
> overwritten by verify_group_input(), and mdata_blocks is read only on the
> ext4_resize_fs() path, which builds its own group_data array.  Copy the
> UAPI size into a zero-initialised struct.
> 
> Reproducible on v6.12.9 and v7.2.4 by placing a 40-byte struct at the end
> of a mapped page whose successor is unmapped: the ioctl returns EFAULT.
> 
> Signed-off-by: Kaixuan Li <kaixuanli0131@xxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
> v3: initialise input with = {} and drop the tail memset (Jan Kara).
>     Correct the description: the UAPI 'unused' shares its slot with
>     mdata_blocks, so only free_clusters_count lies past the copy.
> 
> v2: https://lore.kernel.org/linux-ext4/20260918111742.827428-1-kaixuan.li@xxxxxxxxxx/
> v1: https://lore.kernel.org/linux-ext4/20260917151441.413135-1-kaixuan.li@xxxxxxxxxx/
> 
> Built fs/ext4/ioctl.o warning-free with W=1 on x86_64 (gcc 11.4), and
> checkpatch-clean.  The EFAULT reproducer behaves as before on v6.12.9 and
> v7.2.4; the fix itself was not runtime-tested.
> 
>  fs/ext4/ioctl.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index c8387e6a2c6e..aab02575d90a 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -1674,10 +1674,10 @@ static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	}
>  
>  	case EXT4_IOC_GROUP_ADD: {
> -		struct ext4_new_group_data input;
> +		struct ext4_new_group_input __user *uinput = (void __user *)arg;
> +		struct ext4_new_group_data input = {};
>  
> -		if (copy_from_user(&input, (struct ext4_new_group_input __user *)arg,
> -				sizeof(input)))
> +		if (copy_from_user(&input, uinput, sizeof(*uinput)))
>  			return -EFAULT;
>  
>  		return ext4_ioctl_group_add(filp, &input);
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR