From: Fan Wu <wufan@linux.microsoft.com>
To: Eric Biggers <ebiggers@kernel.org>
Cc: corbet@lwn.net, zohar@linux.ibm.com, jmorris@namei.org,
serge@hallyn.com, tytso@mit.edu, axboe@kernel.dk, agk@redhat.com,
snitzer@kernel.org, eparis@redhat.com, paul@paul-moore.com,
linux-doc@vger.kernel.org, linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org, fsverity@lists.linux.dev,
linux-block@vger.kernel.org, dm-devel@lists.linux.dev,
audit@vger.kernel.org, linux-kernel@vger.kernel.org,
Deven Bowers <deven.desai@linux.microsoft.com>
Subject: Re: [PATCH v17 13/21] dm verity: consume root hash digest and expose signature data via LSM hook
Date: Thu, 25 Apr 2024 13:23:57 -0700 [thread overview]
Message-ID: <6cf278b3-32f2-4665-be8d-ea6605f4318b@linux.microsoft.com> (raw)
In-Reply-To: <20240425035647.GC1401@sol.localdomain>
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
next prev parent reply other threads:[~2024-04-25 20:23 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-13 0:55 [PATCH v17 00/21] Integrity Policy Enforcement LSM (IPE) Fan Wu
2024-04-13 0:55 ` [PATCH v17 01/21] security: add ipe lsm Fan Wu
2024-04-13 0:55 ` [PATCH v17 02/21] ipe: add policy parser Fan Wu
2024-04-13 0:55 ` [PATCH v17 03/21] ipe: add evaluation loop Fan Wu
2024-04-13 0:55 ` [PATCH v17 04/21] ipe: add LSM hooks on execution and kernel read Fan Wu
2024-04-13 0:55 ` [PATCH v17 05/21] initramfs|security: Add a security hook to do_populate_rootfs() Fan Wu
2024-04-13 0:55 ` [PATCH v17 06/21] ipe: introduce 'boot_verified' as a trust provider Fan Wu
2024-04-13 0:55 ` [PATCH v17 07/21] security: add new securityfs delete function Fan Wu
2024-04-13 0:55 ` [PATCH v17 08/21] ipe: add userspace interface Fan Wu
2024-04-13 0:55 ` [PATCH v17 09/21] uapi|audit|ipe: add ipe auditing support Fan Wu
2024-04-13 0:55 ` [PATCH v17 10/21] ipe: add permissive toggle Fan Wu
2024-04-13 0:55 ` [PATCH v17 11/21] block,lsm: add LSM blob and new LSM hooks for block device Fan Wu
2024-04-13 0:55 ` [PATCH v17 12/21] dm: add finalize hook to target_type Fan Wu
2024-04-13 0:55 ` [PATCH v17 13/21] dm verity: consume root hash digest and expose signature data via LSM hook Fan Wu
2024-04-25 3:56 ` Eric Biggers
2024-04-25 20:23 ` Fan Wu [this message]
2024-04-13 0:55 ` [PATCH v17 14/21] ipe: add support for dm-verity as a trust provider Fan Wu
2024-04-13 0:55 ` [PATCH v17 15/21] security: add security_inode_setintegrity() hook Fan Wu
2024-04-13 0:55 ` [PATCH v17 16/21] fsverity: expose verified fsverity built-in signatures to LSMs Fan Wu
2024-04-25 3:36 ` Eric Biggers
2024-04-13 0:56 ` [PATCH v17 17/21] ipe: enable support for fs-verity as a trust provider Fan Wu
2024-04-25 3:42 ` Eric Biggers
2024-04-25 4:20 ` Eric Biggers
2024-04-13 0:56 ` [PATCH v17 18/21] scripts: add boot policy generation program Fan Wu
2024-04-13 0:56 ` [PATCH v17 19/21] ipe: kunit test for parser Fan Wu
2024-04-13 0:56 ` [PATCH v17 20/21] Documentation: add ipe documentation Fan Wu
2024-04-15 12:11 ` Bagas Sanjaya
2024-04-15 14:56 ` Randy Dunlap
2024-04-17 10:05 ` Bagas Sanjaya
2024-04-25 4:13 ` Eric Biggers
2024-04-25 4:36 ` Eric Biggers
2024-04-13 0:56 ` [PATCH v17 21/21] MAINTAINERS: ipe: add ipe maintainer information Fan Wu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6cf278b3-32f2-4665-be8d-ea6605f4318b@linux.microsoft.com \
--to=wufan@linux.microsoft.com \
--cc=agk@redhat.com \
--cc=audit@vger.kernel.org \
--cc=axboe@kernel.dk \
--cc=corbet@lwn.net \
--cc=deven.desai@linux.microsoft.com \
--cc=dm-devel@lists.linux.dev \
--cc=ebiggers@kernel.org \
--cc=eparis@redhat.com \
--cc=fsverity@lists.linux.dev \
--cc=jmorris@namei.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=paul@paul-moore.com \
--cc=serge@hallyn.com \
--cc=snitzer@kernel.org \
--cc=tytso@mit.edu \
--cc=zohar@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).