2022-11-10 20:17:38

by Darrick J. Wong

Subject: [PATCHSET 0/2] ext4: minor fixes to GETFSUUID ioctl

Hi all,

Ted and I looked at the EXT4_IOC_GETFSUUID implementation on the ext4
concall this morning, and I pointed out that there were a couple of odd
things about the ioctl behavior. Since Ted hasn't released a version of
e2fsprogs that uses this ioctl, let's tidy those things up before 6.1
comes out, eh?

This is an extraordinary way to destroy everything. Enjoy!
Comments and questions are, as always, welcome.

--D
---
fs/ext4/ioctl.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)

2022-11-10 20:17:43

by Darrick J. Wong

Subject: [PATCH 1/2] ext4: dont return EINVAL from GETFSUUID when reporting UUID length

From: Darrick J. Wong <[email protected]>

If userspace calls this ioctl with fsu_length (the length of the
fsuuid.fsu_uuid array) set to zero, ext4 copies the desired uuid length
out to userspace. The kernel call returned a result from a valid input,
so the return value here should be zero, not EINVAL.

While we're at it, fix the copy_to_user call to make it clear that we're
only copying out fsu_len.

Signed-off-by: Darrick J. Wong <[email protected]>
---
fs/ext4/ioctl.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)


diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 95dfea28bf4e..5f91f3ad3e50 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -1153,9 +1153,10 @@ static int ext4_ioctl_getuuid(struct ext4_sb_info *sbi,

if (fsuuid.fsu_len == 0) {
fsuuid.fsu_len = UUID_SIZE;
- if (copy_to_user(ufsuuid, &fsuuid, sizeof(fsuuid.fsu_len)))
+ if (copy_to_user(&ufsuuid->fsu_len, &fsuuid.fsu_len,
+ sizeof(fsuuid.fsu_len)))
return -EFAULT;
- return -EINVAL;
+ return 0;
}

if (fsuuid.fsu_len != UUID_SIZE || fsuuid.fsu_flags != 0)

2022-11-10 20:18:12

by Darrick J. Wong

Subject: [PATCH 2/2] ext4: don't fail GETFSUUID when the caller provides a long buffer

From: Darrick J. Wong <[email protected]>

If userspace provides a longer UUID buffer than is required, we
shouldn't fail the call with EINVAL -- rather, we can fill the caller's
buffer with the bytes we /can/ fill, and update the length field to
reflect what we copied. This doesn't break the UAPI since we're
enabling a case that currently fails, and so far Ted hasn't released a
version of e2fsprogs that uses the new ext4 ioctl.

Signed-off-by: Darrick J. Wong <[email protected]>
---
fs/ext4/ioctl.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)


diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 5f91f3ad3e50..31e643795016 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -1159,14 +1159,16 @@ static int ext4_ioctl_getuuid(struct ext4_sb_info *sbi,
return 0;
}

- if (fsuuid.fsu_len != UUID_SIZE || fsuuid.fsu_flags != 0)
+ if (fsuuid.fsu_len < UUID_SIZE || fsuuid.fsu_flags != 0)
return -EINVAL;

lock_buffer(sbi->s_sbh);
memcpy(uuid, sbi->s_es->s_uuid, UUID_SIZE);
unlock_buffer(sbi->s_sbh);

- if (copy_to_user(&ufsuuid->fsu_uuid[0], uuid, UUID_SIZE))
+ fsuuid.fsu_len = UUID_SIZE;
+ if (copy_to_user(ufsuuid, &fsuuid, sizeof(fsuuid)) ||
+ copy_to_user(&ufsuuid->fsu_uuid[0], uuid, UUID_SIZE))
return -EFAULT;
return 0;
}

2022-11-11 01:14:15

by Catherine Hoang

Subject: Re: [PATCH 1/2] ext4: dont return EINVAL from GETFSUUID when reporting UUID length

> On Nov 10, 2022, at 12:16 PM, Darrick J. Wong <[email protected]> wrote:
>
> From: Darrick J. Wong <[email protected]>
>
> If userspace calls this ioctl with fsu_length (the length of the
> fsuuid.fsu_uuid array) set to zero, ext4 copies the desired uuid length
> out to userspace. The kernel call returned a result from a valid input,
> so the return value here should be zero, not EINVAL.
>
> While we're at it, fix the copy_to_user call to make it clear that we're
> only copying out fsu_len.
>
> Signed-off-by: Darrick J. Wong <[email protected]>

Looks good
Reviewed-by: Catherine Hoang <[email protected]>
> ---
> fs/ext4/ioctl.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
>
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 95dfea28bf4e..5f91f3ad3e50 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -1153,9 +1153,10 @@ static int ext4_ioctl_getuuid(struct ext4_sb_info *sbi,
>
> if (fsuuid.fsu_len == 0) {
> fsuuid.fsu_len = UUID_SIZE;
> - if (copy_to_user(ufsuuid, &fsuuid, sizeof(fsuuid.fsu_len)))
> + if (copy_to_user(&ufsuuid->fsu_len, &fsuuid.fsu_len,
> + sizeof(fsuuid.fsu_len)))
> return -EFAULT;
> - return -EINVAL;
> + return 0;
> }
>
> if (fsuuid.fsu_len != UUID_SIZE || fsuuid.fsu_flags != 0)
>

2022-11-11 01:14:30

by Catherine Hoang

Subject: Re: [PATCH 2/2] ext4: don't fail GETFSUUID when the caller provides a long buffer

> On Nov 10, 2022, at 12:16 PM, Darrick J. Wong <[email protected]> wrote:
>
> From: Darrick J. Wong <[email protected]>
>
> If userspace provides a longer UUID buffer than is required, we
> shouldn't fail the call with EINVAL -- rather, we can fill the caller's
> buffer with the bytes we /can/ fill, and update the length field to
> reflect what we copied. This doesn't break the UAPI since we're
> enabling a case that currently fails, and so far Ted hasn't released a
> version of e2fsprogs that uses the new ext4 ioctl.
>
> Signed-off-by: Darrick J. Wong <[email protected]>

Looks good
Reviewed-by: Catherine Hoang <[email protected]>
> ---
> fs/ext4/ioctl.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
>
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 5f91f3ad3e50..31e643795016 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -1159,14 +1159,16 @@ static int ext4_ioctl_getuuid(struct ext4_sb_info *sbi,
> return 0;
> }
>
> - if (fsuuid.fsu_len != UUID_SIZE || fsuuid.fsu_flags != 0)
> + if (fsuuid.fsu_len < UUID_SIZE || fsuuid.fsu_flags != 0)
> return -EINVAL;
>
> lock_buffer(sbi->s_sbh);
> memcpy(uuid, sbi->s_es->s_uuid, UUID_SIZE);
> unlock_buffer(sbi->s_sbh);
>
> - if (copy_to_user(&ufsuuid->fsu_uuid[0], uuid, UUID_SIZE))
> + fsuuid.fsu_len = UUID_SIZE;
> + if (copy_to_user(ufsuuid, &fsuuid, sizeof(fsuuid)) ||
> + copy_to_user(&ufsuuid->fsu_uuid[0], uuid, UUID_SIZE))
> return -EFAULT;
> return 0;
> }
>