Re: [PATCH v17 13/21] dm verity: consume root hash digest and expose signature data via LSM hook

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux