[PATCH] dm-integrity: check mac_size against HASH_MAX_DIGESTSIZE in sb_mac()

[PATCH] dm-integrity: check mac_size against HASH_MAX_DIGESTSIZE in sb_mac()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

sb_mac() verifies that the superblock + MAC don't exceed 512 bytes.
Because the superblock is currently 64 bytes, this really verifies
mac_size <= 448.  This confuses smatch into thinking that mac_size may
be as large as 448, which is inconsistent with the later code that
assumes the MAC fits in a buffer of size HASH_MAX_DIGESTSIZE (64).

In fact mac_size <= HASH_MAX_DIGESTSIZE is guaranteed by the crypto API,
as that is the whole point of HASH_MAX_DIGESTSIZE.  But, let's be
defensive and explicitly check for this.  This suppresses the false
positive smatch warning.  It does not fix an actual bug.

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202409061401.44rtN1bh-lkp@intel.com/
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 drivers/md/dm-integrity.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index 51e6964c13054..3b9738787c855 100644
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -489,11 +489,12 @@ static int sb_mac(struct dm_integrity_c *ic, bool wr)
 	int r;
 	unsigned int mac_size = crypto_shash_digestsize(ic->journal_mac);
 	__u8 *sb = (__u8 *)ic->sb;
 	__u8 *mac = sb + (1 << SECTOR_SHIFT) - mac_size;
 
-	if (sizeof(struct superblock) + mac_size > 1 << SECTOR_SHIFT) {
+	if (sizeof(struct superblock) + mac_size > 1 << SECTOR_SHIFT ||
+	    mac_size > HASH_MAX_DIGESTSIZE) {
 		dm_integrity_io_error(ic, "digest is too long", -EINVAL);
 		return -EINVAL;
 	}
 
 	desc->tfm = ic->journal_mac;

base-commit: 8d8d276ba2fb5f9ac4984f5c10ae60858090babc
-- 
2.46.0

Re: [PATCH] dm-integrity: check mac_size against HASH_MAX_DIGESTSIZE in sb_mac()

[Dan Carpenter]

On Tue, Sep 10, 2024 at 10:52:59AM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> sb_mac() verifies that the superblock + MAC don't exceed 512 bytes.
> Because the superblock is currently 64 bytes, this really verifies
> mac_size <= 448.  This confuses smatch into thinking that mac_size may
> be as large as 448, which is inconsistent with the later code that
> assumes the MAC fits in a buffer of size HASH_MAX_DIGESTSIZE (64).
> 
> In fact mac_size <= HASH_MAX_DIGESTSIZE is guaranteed by the crypto API,
> as that is the whole point of HASH_MAX_DIGESTSIZE.  But, let's be
> defensive and explicitly check for this.  This suppresses the false
> positive smatch warning.  It does not fix an actual bug.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202409061401.44rtN1bh-lkp@intel.com/
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---

This works.  Another option would be to just delete the SECTOR_SIZE check, but
this is obviously more conservative.  ;)

regards,
dan carpenter

Re: [PATCH] dm-integrity: check mac_size against HASH_MAX_DIGESTSIZE in sb_mac()

[Mikulas Patocka]

On Tue, 10 Sep 2024, Eric Biggers wrote:

> From: Eric Biggers <ebiggers@google.com>
> 
> sb_mac() verifies that the superblock + MAC don't exceed 512 bytes.
> Because the superblock is currently 64 bytes, this really verifies
> mac_size <= 448.  This confuses smatch into thinking that mac_size may
> be as large as 448, which is inconsistent with the later code that
> assumes the MAC fits in a buffer of size HASH_MAX_DIGESTSIZE (64).
> 
> In fact mac_size <= HASH_MAX_DIGESTSIZE is guaranteed by the crypto API,
> as that is the whole point of HASH_MAX_DIGESTSIZE.  But, let's be
> defensive and explicitly check for this.  This suppresses the false
> positive smatch warning.  It does not fix an actual bug.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202409061401.44rtN1bh-lkp@intel.com/
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  drivers/md/dm-integrity.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
> index 51e6964c13054..3b9738787c855 100644
> --- a/drivers/md/dm-integrity.c
> +++ b/drivers/md/dm-integrity.c
> @@ -489,11 +489,12 @@ static int sb_mac(struct dm_integrity_c *ic, bool wr)
>  	int r;
>  	unsigned int mac_size = crypto_shash_digestsize(ic->journal_mac);
>  	__u8 *sb = (__u8 *)ic->sb;
>  	__u8 *mac = sb + (1 << SECTOR_SHIFT) - mac_size;
>  
> -	if (sizeof(struct superblock) + mac_size > 1 << SECTOR_SHIFT) {
> +	if (sizeof(struct superblock) + mac_size > 1 << SECTOR_SHIFT ||
> +	    mac_size > HASH_MAX_DIGESTSIZE) {
>  		dm_integrity_io_error(ic, "digest is too long", -EINVAL);
>  		return -EINVAL;
>  	}
>  
>  	desc->tfm = ic->journal_mac;
> 
> base-commit: 8d8d276ba2fb5f9ac4984f5c10ae60858090babc
> -- 
> 2.46.0

I applied the patch.

Mikulas