Re: drivers/md/dm-integrity.c:521 sb_mac() error: __builtin_memcmp() 'actual_mac' too small (64 vs 448)

[Eric Biggers <ebiggers@kernel.org>]

[+Cc dm-devel@lists.linux.dev]

On Tue, Sep 10, 2024 at 10:31:56AM +0300, Dan Carpenter wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head:   b831f83e40a24f07c8dcba5be408d93beedc820f
> commit: 070bb43ab01e891db1b742d4ddd7291c7f8d7022 dm integrity: use crypto_shash_digest() in sb_mac()

This commit seems unrelated, as the alleged issue existed in the code before
that commit too (maybe smatch just didn't notice it yet).

> date:   10 months ago
> config: i386-randconfig-141-20240906 (https://download.01.org/0day-ci/archive/20240906/202409061401.44rtN1bh-lkp@intel.com/config)
> compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | 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/
> 
> smatch warnings:
> drivers/md/dm-integrity.c:521 sb_mac() error: __builtin_memcmp() 'actual_mac' too small (64 vs 448)
> 
> vim +/actual_mac +521 drivers/md/dm-integrity.c
> 
> 09d85f8d8909ec Mikulas Patocka   2021-01-21  492  static int sb_mac(struct dm_integrity_c *ic, bool wr)
> 09d85f8d8909ec Mikulas Patocka   2021-01-21  493  {
> 09d85f8d8909ec Mikulas Patocka   2021-01-21  494  	SHASH_DESC_ON_STACK(desc, ic->journal_mac);
> 09d85f8d8909ec Mikulas Patocka   2021-01-21  495  	int r;
> 070bb43ab01e89 Eric Biggers      2023-10-28  496  	unsigned int mac_size = crypto_shash_digestsize(ic->journal_mac);
> 070bb43ab01e89 Eric Biggers      2023-10-28  497  	__u8 *sb = (__u8 *)ic->sb;
> 070bb43ab01e89 Eric Biggers      2023-10-28  498  	__u8 *mac = sb + (1 << SECTOR_SHIFT) - mac_size;
> 09d85f8d8909ec Mikulas Patocka   2021-01-21  499  
> 070bb43ab01e89 Eric Biggers      2023-10-28  500  	if (sizeof(struct superblock) + mac_size > 1 << SECTOR_SHIFT) {
> 
> This is paired with the line before and prevents the subtraction from going
> negative.  It limits the mac_size to 0-448.  Is it reasonable to have a mac_size
> which is > HASH_MAX_DIGESTSIZE (64)?

crypto_shash_digestsize() cannot return a value greater than HASH_MAX_DIGESTSIZE
because the crypto API doesn't allow registering any hash algorithms with
digests larger than that.  That's the whole point of HASH_MAX_DIGESTSIZE.

> This buffer is only 64 bytes.

Yes.

> 0ef0b4717aa684 Heinz Mauelshagen 2023-02-01  515  
> 070bb43ab01e89 Eric Biggers      2023-10-28  516  		r = crypto_shash_digest(desc, sb, mac - sb, actual_mac);
> 09d85f8d8909ec Mikulas Patocka   2021-01-21  517  		if (unlikely(r < 0)) {
> 070bb43ab01e89 Eric Biggers      2023-10-28  518  			dm_integrity_io_error(ic, "crypto_shash_digest", r);
> 09d85f8d8909ec Mikulas Patocka   2021-01-21  519  			return r;
> 09d85f8d8909ec Mikulas Patocka   2021-01-21  520  		}
> 070bb43ab01e89 Eric Biggers      2023-10-28 @521  		if (memcmp(mac, actual_mac, mac_size)) {
>                                                                                 ^^^^^^^^^^
> Read overflow.

No, because mac_size <= 64.

We might as well explicitly check that in the code to suppress the static
analysis warning (I'll send a patch), but it's not fixing an actual bug.

- Eric