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

[Dan Carpenter]

On Tue, Sep 10, 2024 at 10:20:01AM -0700, Eric Biggers wrote:
> [+Cc dm-devel@xxxxxxxxxxxxxxx]
> 
> 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@xxxxxxxxx/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@xxxxxxxxx>
> > | Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > | Closes: https://lore.kernel.org/r/202409061401.44rtN1bh-lkp@xxxxxxxxx/
> > 
> > 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.

Generally, we try avoid silencing warnings just for silencing them unless it
makes the code more readable.

The other way to silence this warning would be to delete the check on line 500
because if it can't be larger than 64 then it can't be larger than 448.  It's
not like SECTOR_SIZE is going to get smaller in the future.

regards,
dan carpenter