fuse: fs-verity: aoid out-of-range comparison

[PATCH] fuse: fs-verity: aoid out-of-range comparison

Posted by Arnd Bergmann 1 year, 4 months ago

From: Arnd Bergmann <arnd@arndb.de>

clang points out that comparing the 16-bit size of the digest against
SIZE_MAX is not a helpful comparison:

fs/fuse/ioctl.c:130:18: error: result of comparison of constant 18446744073709551611 with expression of type '__u16' (aka 'unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
        if (digest_size > SIZE_MAX - sizeof(struct fsverity_digest))
            ~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This either means tha tthe check can be removed entirely, or that the
intended comparison was for the 16-bit range. Assuming the latter was
intended, compare against U16_MAX instead.

Fixes: 9fe2a036a23c ("fuse: Add initial support for fs-verity")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/fuse/ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/fuse/ioctl.c b/fs/fuse/ioctl.c
index 572ce8a82ceb..5711d86c549d 100644
--- a/fs/fuse/ioctl.c
+++ b/fs/fuse/ioctl.c
@@ -127,7 +127,7 @@ static int fuse_setup_measure_verity(unsigned long arg, struct iovec *iov)
 	if (copy_from_user(&digest_size, &uarg->digest_size, sizeof(digest_size)))
 		return -EFAULT;
 
-	if (digest_size > SIZE_MAX - sizeof(struct fsverity_digest))
+	if (digest_size > U16_MAX - sizeof(struct fsverity_digest))
 		return -EINVAL;
 
 	iov->iov_len = sizeof(struct fsverity_digest) + digest_size;
-- 
2.39.2

Re: [PATCH] fuse: fs-verity: aoid out-of-range comparison

Posted by Eric Biggers 1 year, 4 months ago

On Tue, Jul 30, 2024 at 04:27:52PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> clang points out that comparing the 16-bit size of the digest against
> SIZE_MAX is not a helpful comparison:
> 
> fs/fuse/ioctl.c:130:18: error: result of comparison of constant 18446744073709551611 with expression of type '__u16' (aka 'unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
>         if (digest_size > SIZE_MAX - sizeof(struct fsverity_digest))
>             ~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> This either means tha tthe check can be removed entirely, or that the
> intended comparison was for the 16-bit range. Assuming the latter was
> intended, compare against U16_MAX instead.
> 
> Fixes: 9fe2a036a23c ("fuse: Add initial support for fs-verity")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  fs/fuse/ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/fuse/ioctl.c b/fs/fuse/ioctl.c
> index 572ce8a82ceb..5711d86c549d 100644
> --- a/fs/fuse/ioctl.c
> +++ b/fs/fuse/ioctl.c
> @@ -127,7 +127,7 @@ static int fuse_setup_measure_verity(unsigned long arg, struct iovec *iov)
>  	if (copy_from_user(&digest_size, &uarg->digest_size, sizeof(digest_size)))
>  		return -EFAULT;
>  
> -	if (digest_size > SIZE_MAX - sizeof(struct fsverity_digest))
> +	if (digest_size > U16_MAX - sizeof(struct fsverity_digest))
>  		return -EINVAL;
>  
>  	iov->iov_len = sizeof(struct fsverity_digest) + digest_size;

I think this was just defensive coding to ensure that the assignment to iov_len
can't overflow regardless of the type of digest_size.  You can remove the check
if you want to, though isn't the tautological comparison warning disabled by the
kernel build system anyway?  Anyway, it does not make sense to use U16_MAX here.

- Eric

Re: [PATCH] fuse: fs-verity: aoid out-of-range comparison

Posted by Richard Fung 1 year, 4 months ago

> I think this was just defensive coding to ensure that the assignment to iov_len can't overflow regardless of the type of digest_size

I agree with this. Removing it sounds good to me

Re: [PATCH] fuse: fs-verity: aoid out-of-range comparison

Posted by Nathan Chancellor 1 year, 4 months ago

On Tue, Jul 30, 2024 at 04:27:52PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> clang points out that comparing the 16-bit size of the digest against
> SIZE_MAX is not a helpful comparison:
> 
> fs/fuse/ioctl.c:130:18: error: result of comparison of constant 18446744073709551611 with expression of type '__u16' (aka 'unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
>         if (digest_size > SIZE_MAX - sizeof(struct fsverity_digest))
>             ~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> This either means tha tthe check can be removed entirely, or that the
> intended comparison was for the 16-bit range. Assuming the latter was
> intended, compare against U16_MAX instead.

Presumably this check was added because of the addition in the
assignment to iov_len, which is size_t, but I don't see how that
expression could realistically overflow? It seems like this whole check
could just be removed?

> Fixes: 9fe2a036a23c ("fuse: Add initial support for fs-verity")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  fs/fuse/ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/fuse/ioctl.c b/fs/fuse/ioctl.c
> index 572ce8a82ceb..5711d86c549d 100644
> --- a/fs/fuse/ioctl.c
> +++ b/fs/fuse/ioctl.c
> @@ -127,7 +127,7 @@ static int fuse_setup_measure_verity(unsigned long arg, struct iovec *iov)
>  	if (copy_from_user(&digest_size, &uarg->digest_size, sizeof(digest_size)))
>  		return -EFAULT;
>  
> -	if (digest_size > SIZE_MAX - sizeof(struct fsverity_digest))
> +	if (digest_size > U16_MAX - sizeof(struct fsverity_digest))
>  		return -EINVAL;
>  
>  	iov->iov_len = sizeof(struct fsverity_digest) + digest_size;
> -- 
> 2.39.2
>