* drivers/md/dm-integrity.c:521 sb_mac() error: __builtin_memcmp() 'actual_mac' too small (64 vs 448)
@ 2024-09-10 7:31 Dan Carpenter
2024-09-10 17:20 ` Eric Biggers
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2024-09-10 7:31 UTC (permalink / raw)
To: oe-kbuild, Eric Biggers; +Cc: lkp, oe-kbuild-all, linux-kernel, Mike Snitzer
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()
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)?
09d85f8d8909ec Mikulas Patocka 2021-01-21 501 dm_integrity_io_error(ic, "digest is too long", -EINVAL);
09d85f8d8909ec Mikulas Patocka 2021-01-21 502 return -EINVAL;
09d85f8d8909ec Mikulas Patocka 2021-01-21 503 }
09d85f8d8909ec Mikulas Patocka 2021-01-21 504
09d85f8d8909ec Mikulas Patocka 2021-01-21 505 desc->tfm = ic->journal_mac;
09d85f8d8909ec Mikulas Patocka 2021-01-21 506
09d85f8d8909ec Mikulas Patocka 2021-01-21 507 if (likely(wr)) {
070bb43ab01e89 Eric Biggers 2023-10-28 508 r = crypto_shash_digest(desc, sb, mac - sb, mac);
09d85f8d8909ec Mikulas Patocka 2021-01-21 509 if (unlikely(r < 0)) {
070bb43ab01e89 Eric Biggers 2023-10-28 510 dm_integrity_io_error(ic, "crypto_shash_digest", r);
09d85f8d8909ec Mikulas Patocka 2021-01-21 511 return r;
09d85f8d8909ec Mikulas Patocka 2021-01-21 512 }
09d85f8d8909ec Mikulas Patocka 2021-01-21 513 } else {
070bb43ab01e89 Eric Biggers 2023-10-28 514 __u8 actual_mac[HASH_MAX_DIGESTSIZE];
This buffer is only 64 bytes.
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.
09d85f8d8909ec Mikulas Patocka 2021-01-21 522 dm_integrity_io_error(ic, "superblock mac", -EILSEQ);
82bb85998cc9a3 Michael Weiß 2021-09-04 523 dm_audit_log_target(DM_MSG_PREFIX, "mac-superblock", ic->ti, 0);
09d85f8d8909ec Mikulas Patocka 2021-01-21 524 return -EILSEQ;
09d85f8d8909ec Mikulas Patocka 2021-01-21 525 }
09d85f8d8909ec Mikulas Patocka 2021-01-21 526 }
09d85f8d8909ec Mikulas Patocka 2021-01-21 527
09d85f8d8909ec Mikulas Patocka 2021-01-21 528 return 0;
09d85f8d8909ec Mikulas Patocka 2021-01-21 529 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: drivers/md/dm-integrity.c:521 sb_mac() error: __builtin_memcmp() 'actual_mac' too small (64 vs 448)
2024-09-10 7:31 drivers/md/dm-integrity.c:521 sb_mac() error: __builtin_memcmp() 'actual_mac' too small (64 vs 448) Dan Carpenter
@ 2024-09-10 17:20 ` Eric Biggers
2024-09-10 18:41 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Eric Biggers @ 2024-09-10 17:20 UTC (permalink / raw)
To: Dan Carpenter
Cc: oe-kbuild, lkp, oe-kbuild-all, linux-kernel, Mike Snitzer,
dm-devel
[+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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: drivers/md/dm-integrity.c:521 sb_mac() error: __builtin_memcmp() 'actual_mac' too small (64 vs 448)
2024-09-10 17:20 ` Eric Biggers
@ 2024-09-10 18:41 ` Dan Carpenter
2024-09-10 18:53 ` Eric Biggers
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2024-09-10 18:41 UTC (permalink / raw)
To: Eric Biggers
Cc: oe-kbuild, lkp, oe-kbuild-all, linux-kernel, Mike Snitzer,
dm-devel
On Tue, Sep 10, 2024 at 10:20:01AM -0700, Eric Biggers wrote:
> [+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.
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: drivers/md/dm-integrity.c:521 sb_mac() error: __builtin_memcmp() 'actual_mac' too small (64 vs 448)
2024-09-10 18:41 ` Dan Carpenter
@ 2024-09-10 18:53 ` Eric Biggers
0 siblings, 0 replies; 5+ messages in thread
From: Eric Biggers @ 2024-09-10 18:53 UTC (permalink / raw)
To: Dan Carpenter
Cc: oe-kbuild, lkp, oe-kbuild-all, linux-kernel, Mike Snitzer,
dm-devel
On Tue, Sep 10, 2024 at 09:41:34PM +0300, Dan Carpenter wrote:
> 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.
But maybe sizeof(struct superblock) could get larger in the future.
- Eric
^ permalink raw reply [flat|nested] 5+ messages in thread
* drivers/md/dm-integrity.c:521 sb_mac() error: __builtin_memcmp() 'actual_mac' too small (64 vs 448)
@ 2024-01-10 6:52 Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2024-01-10 6:52 UTC (permalink / raw)
To: oe-kbuild, Eric Biggers; +Cc: lkp, oe-kbuild-all, linux-kernel, Mike Snitzer
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head: 9f8413c4a66f2fb776d3dc3c9ed20bf435eb305e
commit: 070bb43ab01e891db1b742d4ddd7291c7f8d7022 dm integrity: use crypto_shash_digest() in sb_mac()
config: i386-randconfig-141-20240107 (https://download.01.org/0day-ci/archive/20240109/202401092335.ZFBXDPA1-lkp@intel.com/config)
compiler: ClangBuiltLinux clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
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/202401092335.ZFBXDPA1-lkp@intel.com/
New smatch warnings:
drivers/md/dm-integrity.c:521 sb_mac() error: __builtin_memcmp() 'actual_mac' too small (64 vs 448)
Old smatch warnings:
drivers/md/dm-integrity.c:3607 dm_integrity_alloc_page_list() warn: Please consider using kvcalloc instead of kvmalloc_array
drivers/md/dm-integrity.c:3641 dm_integrity_alloc_journal_scatterlist() warn: Please consider using kvcalloc instead of kvmalloc_array
drivers/md/dm-integrity.c:3910 create_journal() warn: Please consider using kvcalloc instead of kvmalloc_array
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 caps mac_size to 448.
09d85f8d8909ec Mikulas Patocka 2021-01-21 501 dm_integrity_io_error(ic, "digest is too long", -EINVAL);
09d85f8d8909ec Mikulas Patocka 2021-01-21 502 return -EINVAL;
09d85f8d8909ec Mikulas Patocka 2021-01-21 503 }
09d85f8d8909ec Mikulas Patocka 2021-01-21 504
09d85f8d8909ec Mikulas Patocka 2021-01-21 505 desc->tfm = ic->journal_mac;
09d85f8d8909ec Mikulas Patocka 2021-01-21 506
09d85f8d8909ec Mikulas Patocka 2021-01-21 507 if (likely(wr)) {
070bb43ab01e89 Eric Biggers 2023-10-28 508 r = crypto_shash_digest(desc, sb, mac - sb, mac);
09d85f8d8909ec Mikulas Patocka 2021-01-21 509 if (unlikely(r < 0)) {
070bb43ab01e89 Eric Biggers 2023-10-28 510 dm_integrity_io_error(ic, "crypto_shash_digest", r);
09d85f8d8909ec Mikulas Patocka 2021-01-21 511 return r;
09d85f8d8909ec Mikulas Patocka 2021-01-21 512 }
09d85f8d8909ec Mikulas Patocka 2021-01-21 513 } else {
070bb43ab01e89 Eric Biggers 2023-10-28 514 __u8 actual_mac[HASH_MAX_DIGESTSIZE];
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)) {
But if it's more than 64 then it's a read overflow.
09d85f8d8909ec Mikulas Patocka 2021-01-21 522 dm_integrity_io_error(ic, "superblock mac", -EILSEQ);
82bb85998cc9a3 Michael Weiß 2021-09-04 523 dm_audit_log_target(DM_MSG_PREFIX, "mac-superblock", ic->ti, 0);
09d85f8d8909ec Mikulas Patocka 2021-01-21 524 return -EILSEQ;
09d85f8d8909ec Mikulas Patocka 2021-01-21 525 }
09d85f8d8909ec Mikulas Patocka 2021-01-21 526 }
09d85f8d8909ec Mikulas Patocka 2021-01-21 527
09d85f8d8909ec Mikulas Patocka 2021-01-21 528 return 0;
09d85f8d8909ec Mikulas Patocka 2021-01-21 529 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-09-10 18:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-10 7:31 drivers/md/dm-integrity.c:521 sb_mac() error: __builtin_memcmp() 'actual_mac' too small (64 vs 448) Dan Carpenter
2024-09-10 17:20 ` Eric Biggers
2024-09-10 18:41 ` Dan Carpenter
2024-09-10 18:53 ` Eric Biggers
-- strict thread matches above, loose matches on Subject: below --
2024-01-10 6:52 Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox