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.
> 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
> +#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...
> 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
