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

[Andreas Dilger <adilger@dilger.ca>]

On Sep 17, 2026, at 09:19, MarkLee131 <kaixuanli0131@gmail.com> wrote:
> 
> From: Kaixuan Li <kaixuanli0131@gmail.com>
> 
> 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 internal struct
> has carried the extra fields since ext4 was split from ext3, so the copy
> has always over-read the UAPI object.
> 
> The two extra fields are not used from this path: free_clusters_count is
> overwritten by verify_group_input(), and mdata_blocks is used only by
> ext4_resize_fs(), which builds its own group_data array.  The compat path
> already copies the six UAPI fields individually; the native path does not.
> 
> Copy the UAPI struct, then set the internal fields from it.
> 
> 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@gmail.com>
> ---
> Built fs/ext4/ioctl.o warning-free (W=1, x86_64 defconfig) and checkpatch-clean.
> The bug (EFAULT on a conforming 40-byte object) was reproduced under QEMU on
> v6.12.9 and v7.2.4; the fix itself was not runtime-tested.
> fs/ext4/ioctl.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index c8387e6a2c6e..ea3cd8cdae25 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -1674,12 +1674,22 @@ static long __ext4_ioctl(struct file *filp, unsigned int cmd,
>  	}
>  
>  	case EXT4_IOC_GROUP_ADD: {
> +		struct ext4_new_group_input uinput;
>  		struct ext4_new_group_data input;
>  
> -		if (copy_from_user(&input, (struct ext4_new_group_input __user *)arg,
> -				sizeof(input)))
> +		if (copy_from_user(&uinput,
> +				   (struct ext4_new_group_input __user *)arg,
> +				   sizeof(uinput)))
>  			return -EFAULT;
>  
> +		memset(&input, 0, sizeof(input));
> +		input.group = uinput.group;
> +		input.block_bitmap = uinput.block_bitmap;
> +		input.inode_bitmap = uinput.inode_bitmap;
> +		input.inode_table = uinput.inode_table;
> +		input.blocks_count = uinput.blocks_count;
> +		input.reserved_blocks = uinput.reserved_blocks;
> +

This does more than necessary.  It doesn't need two copies of the struct on the stack,
and it doesn't need to copy the fields twice.  It could just copy the 'input' part of
the struct into the 'data' struct and zero only the remaining fields, something like:

	case EXT4_IOC_GROUP_ADD: {
		struct ext4_new_group_input __user *uinput = (void __user *)arg;
		struct ext4_new_group_data data;
 
		if (copy_from_user(&data, uinput, sizeof(*uinput))
			return -EFAULT;
 
		memset(&data + sizeof(*uinput), 0, sizeof(data) - sizeof(*uinput));

		return ext4_ioctl_group_add(filp, &data);

Cheers, Andreas