| Message ID | 20240910175259.28620-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Mikulas Patocka |
| Headers | show |
| Series | dm-integrity: check mac_size against HASH_MAX_DIGESTSIZE in sb_mac() | expand |
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
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
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;