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 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 identical layout, so copying
that much fills every field the UAPI defines.  Zero the rest:
free_clusters_count is overwritten by verify_group_input() anyway, and
mdata_blocks is read only by ext4_resize_fs(), which builds its own
group_data array.

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>
---
v2: done the way you suggested -- one struct on the stack, one copy, and a
memset of the tail, rather than v1's second struct and six field
assignments.  The only change from your sketch is the cast in the memset
address: &input is a struct ext4_new_group_data *, so &input +
sizeof(*uinput) would advance by 40 * 48 bytes.

Built fs/ext4/ioctl.o warning-free with W=1 on x86_64 (clang 21), 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 | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -1674,12 +1674,15 @@
 	}
 
 	case EXT4_IOC_GROUP_ADD: {
+		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;
 
+		memset((char *)&input + sizeof(*uinput), 0,
+		       sizeof(input) - sizeof(*uinput));
+
 		return ext4_ioctl_group_add(filp, &input);
 	}

On Fri 18-09-26 19:17:42, MarkLee131 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 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 identical layout, so copying
> that much fills every field the UAPI defines.  Zero the rest:
> free_clusters_count is overwritten by verify_group_input() anyway, and
> mdata_blocks is read only by ext4_resize_fs(), which builds its own
> group_data array.
> 
> 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>
> ---
> v2: done the way you suggested -- one struct on the stack, one copy, and a
> memset of the tail, rather than v1's second struct and six field
> assignments.  The only change from your sketch is the cast in the memset
> address: &input is a struct ext4_new_group_data *, so &input +
> sizeof(*uinput) would advance by 40 * 48 bytes.
> 
> Built fs/ext4/ioctl.o warning-free with W=1 on x86_64 (clang 21), and
> checkpatch-clean.  The EFAULT reproducer behaves as before on v6.12.9 and
> v7.2.4; the fix itself was not runtime-tested.

One nit below, otherwise looks good.

> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -1674,12 +1674,15 @@
>  	}
>  
>  	case EXT4_IOC_GROUP_ADD: {
> +		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;
>  
> +		memset((char *)&input + sizeof(*uinput), 0,
> +		       sizeof(input) - sizeof(*uinput));
> +

I just would not bother with this complex memset. This is not performance
critical at all. Just initialize the whole 'input' to 0 (with = {} at
declaration) and be done with it.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR