Messages in this thread | | | Date | Thu, 25 Apr 2024 13:23:57 -0700 | Subject | Re: [PATCH v17 13/21] dm verity: consume root hash digest and expose signature data via LSM hook | From | Fan Wu <> |
| |
On 4/24/2024 8:56 PM, Eric Biggers wrote: > On Fri, Apr 12, 2024 at 05:55:56PM -0700, Fan Wu wrote: >> dm verity: consume root hash digest and expose signature data via LSM hook > > As in the fsverity patch, nothing is being "consumed" here. This patch adds a > supplier, not a consumer. I think you mean something like: expose root digest > and signature to LSMs. > Thanks for the suggestion.
>> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c >> index bb5da66da4c1..fbb83c6fd99c 100644 >> --- a/drivers/md/dm-verity-target.c >> +++ b/drivers/md/dm-verity-target.c >> @@ -22,6 +22,8 @@ >> #include <linux/scatterlist.h> >> #include <linux/string.h> >> #include <linux/jump_label.h> >> +#include <linux/security.h> >> +#include <linux/dm-verity.h> >> >> #define DM_MSG_PREFIX "verity" >> >> @@ -1017,6 +1019,38 @@ static void verity_io_hints(struct dm_target *ti, struct queue_limits *limits) >> blk_limits_io_min(limits, limits->logical_block_size); >> } >> >> +#ifdef CONFIG_SECURITY >> + >> +static int verity_init_sig(struct dm_verity *v, const void *sig, >> + size_t sig_size) >> +{ >> + v->sig_size = sig_size; >> + v->root_digest_sig = kmemdup(sig, v->sig_size, GFP_KERNEL); >> + if (!v->root_digest) >> + return -ENOMEM; > > root_digest_sig, not root_digest > Thanks for pointing out!
>> +#ifdef CONFIG_SECURITY >> + >> +static int verity_finalize(struct dm_target *ti) >> +{ >> + struct block_device *bdev; >> + struct dm_verity_digest root_digest; >> + struct dm_verity *v; >> + int r; >> + >> + v = ti->private; >> + bdev = dm_disk(dm_table_get_md(ti->table))->part0; >> + root_digest.digest = v->root_digest; >> + root_digest.digest_len = v->digest_size; >> + root_digest.alg = v->alg_name; >> + >> + r = security_bdev_setintegrity(bdev, LSM_INT_DMVERITY_ROOTHASH, &root_digest, >> + sizeof(root_digest)); >> + if (r) >> + return r; >> + >> + r = security_bdev_setintegrity(bdev, >> + LSM_INT_DMVERITY_SIG_VALID, >> + v->root_digest_sig, >> + v->sig_size); > > The signature is only checked if CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG=y, whereas > this code is built whenever CONFIG_SECURITY=y. > > So this seems like the same issue that has turned up elsewhere in the IPE > patchset, where IPE is (apparently) happy with any signature, even one that > hasn't been checked... >
Yes I do agree the second hook call should better depend on CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG=y.
However, the current implementation does not happy with any signature.
In case of CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG=y, any signature provided to dm-verity will be checked against the configured keyring, the hook call won't be reached if the check failed. In case of no signature is provided and !DM_VERITY_IS_SIG_FORCE_ENABLED(), the hook will be called with signature value NULL.
In case of CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG=n, signature won't be accepted by dm-verity. In addition, the whole support of dm-verity will be disabled for IPE because CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG=n.
>> diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h >> index 20b1bcf03474..89e862f0cdf6 100644 >> --- a/drivers/md/dm-verity.h >> +++ b/drivers/md/dm-verity.h >> @@ -43,6 +43,9 @@ struct dm_verity { >> u8 *root_digest; /* digest of the root block */ >> u8 *salt; /* salt: its size is salt_size */ >> u8 *zero_digest; /* digest for a zero block */ >> +#ifdef CONFIG_SECURITY >> + u8 *root_digest_sig; /* digest signature of the root block */ >> +#endif /* CONFIG_SECURITY */ > > No, it's not a signature of the root block, at least not directly. It's a > signature of the root digest (the digest of the root block). > >> diff --git a/include/linux/dm-verity.h b/include/linux/dm-verity.h >> new file mode 100644 >> index 000000000000..a799a8043d85 >> --- /dev/null >> +++ b/include/linux/dm-verity.h >> @@ -0,0 +1,12 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> + >> +#ifndef _LINUX_DM_VERITY_H >> +#define _LINUX_DM_VERITY_H >> + >> +struct dm_verity_digest { >> + const char *alg; >> + const u8 *digest; >> + size_t digest_len; >> +}; >> + >> +#endif /* _LINUX_DM_VERITY_H */ >> diff --git a/include/linux/security.h b/include/linux/security.h >> index ac0985641611..9e46b13a356c 100644 >> --- a/include/linux/security.h >> +++ b/include/linux/security.h >> @@ -84,7 +84,8 @@ enum lsm_event { >> }; >> >> enum lsm_integrity_type { >> - __LSM_INT_MAX >> + LSM_INT_DMVERITY_SIG_VALID, >> + LSM_INT_DMVERITY_ROOTHASH, >> }; > > Shouldn't struct dm_verity_digest be defined next to LSM_INT_DMVERITY_ROOTHASH? > It's the struct that's associated with it. > > It seems weird to create a brand new header <linux/dm-verity.h> that just > contains this one LSM related definition, when there's already a header for the > LSM definitions that even includes the related value LSM_INT_DMVERITY_ROOTHASH. > > - Eric
Yes they can just be in the same header. Thanks for the suggestion.
-Fan
| |