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

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

[Darrick J. Wong]

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(-)

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

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

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 <djwong@kernel.org>
---
 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)

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

[Catherine Hoang]

> On Nov 10, 2022, at 12:16 PM, Darrick J. Wong <djwong@kernel.org> wrote:
> 
> From: Darrick J. Wong <djwong@kernel.org>
> 
> 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 <djwong@kernel.org>

Looks good
Reviewed-by: Catherine Hoang <catherine.hoang@oracle.com>
> ---
> 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)
> 

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

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

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 <djwong@kernel.org>
---
 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;
 }

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

[Catherine Hoang]

> On Nov 10, 2022, at 12:16 PM, Darrick J. Wong <djwong@kernel.org> wrote:
> 
> From: Darrick J. Wong <djwong@kernel.org>
> 
> 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 <djwong@kernel.org>

Looks good
Reviewed-by: Catherine Hoang <catherine.hoang@oracle.com>
> ---
> 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;
> }
>