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
