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

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

Posted by MarkLee131 1 week ago

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, unsigned long arg)
 	}
 
 	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;
+
 		return ext4_ioctl_group_add(filp, &input);
 	}
 
-- 
2.34.1

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

Posted by MarkLee131 6 days, 14 hours ago

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);
 	}

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

Posted by Jan Kara 2 days, 15 hours ago

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