From: Fan Wu <wufan@linux.microsoft.com>
To: Paul Moore <paul@paul-moore.com>,
corbet@lwn.net, zohar@linux.ibm.com, jmorris@namei.org,
serge@hallyn.com, tytso@mit.edu, ebiggers@kernel.org,
axboe@kernel.dk, agk@redhat.com, snitzer@kernel.org,
eparis@redhat.com
Cc: 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 RFC v15 14/21] dm verity: consume root hash digest and signature data via LSM hook
Date: Wed, 20 Mar 2024 10:56:04 -0700 [thread overview]
Message-ID: <2e76fd2f-f116-4aea-aa6d-4b3feef1c31e@linux.microsoft.com> (raw)
In-Reply-To: <657b73a0cf531fd4291a0f780d2fcf78@paul-moore.com>
On 3/19/2024 4:00 PM, Paul Moore wrote:
> On Mar 15, 2024 Fan Wu <wufan@linux.microsoft.com> wrote:
>>
>> dm-verity provides a strong guarantee of a block device's integrity. As
>> a generic way to check the integrity of a block device, it provides
>> those integrity guarantees to its higher layers, including the filesystem
>> level.
>>
>> An LSM that control access to a resource on the system based on the
>> available integrity claims can use this transitive property of
>> dm-verity, by querying the underlying block_device of a particular
>> file.
>>
>> The digest and signature information need to be stored in the block
>> device to fulfill the next requirement of authorization via LSM policy.
>> This will enable the LSM to perform revocation of devices that are still
>> mounted, prohibiting execution of files that are no longer authorized
>> by the LSM in question.
>>
>> This patch adds two security hook calls in dm-verity to save the
>> dm-verity roothash and the roothash signature to the block device's
>> LSM blobs. The hook calls are depended on CONFIG_IPE_PROP_DM_VERITY,
>> which will be introduced in the next commit.
>>
>> Signed-off-by: Deven Bowers <deven.desai@linux.microsoft.com>
>> Signed-off-by: Fan Wu <wufan@linux.microsoft.com>
>> ---
>> v2:
>> + No Changes
>>
>> v3:
>> + No changes
>>
>> v4:
>> + No changes
>>
>> v5:
>> + No changes
>>
>> v6:
>> + Fix an improper cleanup that can result in
>> a leak
>>
>> v7:
>> + Squash patch 08/12, 10/12 to [11/16]
>> + Use part0 for block_device, to retrieve the block_device, when
>> calling security_bdev_setsecurity
>>
>> v8:
>> + Undo squash of 08/12, 10/12 - separating drivers/md/ from
>> security/ & block/
>> + Use common-audit function for dmverity_signature.
>> + Change implementation for storing the dm-verity digest to use the
>> newly introduced dm_verity_digest structure introduced in patch
>> 14/20.
>> + Create new structure, dm_verity_digest, containing digest algorithm,
>> size, and digest itself to pass to the LSM layer. V7 was missing the
>> algorithm.
>> + Create an associated public header containing this new structure and
>> the key values for the LSM hook, specific to dm-verity.
>> + Additional information added to commit, discussing the layering of
>> the changes and how the information passed will be used.
>>
>> v9:
>> + No changes
>>
>> v10:
>> + No changes
>>
>> v11:
>> + Add an optional field to save signature
>> + Move the security hook call to the new finalize hook
>>
>> v12:
>> + No changes
>>
>> v13:
>> + No changes
>>
>> v14:
>> + Correct code format
>> + Remove unnecessary header and switch to dm_disk()
>>
>> v15:
>> + Refactor security_bdev_setsecurity() to security_bdev_setintegrity()
>> + Remove unnecessary headers
>> ---
>> drivers/md/dm-verity-target.c | 73 +++++++++++++++++++++++++++++++++++
>> drivers/md/dm-verity.h | 6 +++
>> include/linux/dm-verity.h | 12 ++++++
>> include/linux/security.h | 2 +
>> 4 files changed, 93 insertions(+)
>> create mode 100644 include/linux/dm-verity.h
>>
>> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
>> index bb5da66da4c1..e94cc6a755d5 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_IPE_PROP_DM_VERITY
>> +
>> +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 = kmalloc(v->sig_size, GFP_KERNEL);
>> + if (!v->root_digest)
>> + return -ENOMEM;
>
> Either you meant to copy @sig into @v->root_digest_sig and forgot to
> add the code for that, or we don't need to include @sig as a parameter
> to this function. I'm guessing it is the former as it wouldn't make
> sense to even have dm_verity::root_digest_sig if we weren't stashing
> it here.
>
> I'd also suggest looking at kmemdup() instead of a kmalloc()/memcpy()
> combo.
>
Sorry, my mistake here. I must have thought I wrote kmemdup(). I will
fix this.
>> + return 0;
>> +}
>> +
>> +static void verity_free_sig(struct dm_verity *v)
>> +{
>> + kfree(v->root_digest_sig);
>> +}
>> +#else
>> +
>> +static inline int verity_init_sig(struct dm_verity *v, const void *sig,
>> + size_t sig_size)
>> +{
>> + return 0;
>> +}
>> +
>> +static inline void verity_free_sig(struct dm_verity *v)
>> +{
>> +}
>> +
>> +#endif /* CONFIG_IPE_PROP_DM_VERITY */
>
> It's been a while since I looked at this patch in the patchset, so
> maybe I'm missing something, but in general we don't want CONFIG_XXX
> checks in the kernel, outside of security/, that are specific to a
> particular LSM (what happens when multiple LSMs need this?). Please
> use CONFIG_SECURITY instead.
>
>> static void verity_dtr(struct dm_target *ti)
>> {
>> struct dm_verity *v = ti->private;
>> @@ -1035,6 +1069,7 @@ static void verity_dtr(struct dm_target *ti)
>> kfree(v->salt);
>> kfree(v->root_digest);
>> kfree(v->zero_digest);
>> + verity_free_sig(v);
>>
>> if (v->tfm)
>> crypto_free_ahash(v->tfm);
>> @@ -1434,6 +1469,13 @@ static int verity_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>> ti->error = "Root hash verification failed";
>> goto bad;
>> }
>> +
>> + r = verity_init_sig(v, verify_args.sig, verify_args.sig_size);
>> + if (r < 0) {
>> + ti->error = "Cannot allocate root digest signature";
>> + goto bad;
>> + }
>> +
>> v->hash_per_block_bits =
>> __fls((1 << v->hash_dev_block_bits) / v->digest_size);
>>
>> @@ -1584,6 +1626,34 @@ int dm_verity_get_root_digest(struct dm_target *ti, u8 **root_digest, unsigned i
>> return 0;
>> }
>>
>> +#ifdef CONFIG_IPE_PROP_DM_VERITY
>> +
>> +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_INTGR_DMV_ROOTHASH, &root_digest,
>> + sizeof(root_digest));
>> + if (r)
>> + return r;
>> +
>> + return security_bdev_setintegrity(bdev,
>> + LSM_INTGR_DMV_SIG,
>> + v->root_digest_sig,
>> + v->sig_size);
>
> What happens if the second call fails, should we clear the
> LSM_INTGR_DMV_ROOTHASH state in the LSM?
>
Yes, we should also clear the ROOTHASH if the second failed. Probably we
can pass NULL to security_bdev_setintegrity to clear it like
security_bdev_setintegrity(bdev, LSM_INTGR_DMV_ROOTHASH, NULL, 0);
-Fan
>> +}
>> +
>> +#endif /* CONFIG_IPE_PROP_DM_VERITY */
>
> See my comments about CONFIG_SECURITY above. In fact, I would suggest
> moving this up into that part of the file so you only need one #ifdef
> block relating to CONFIG_SECURITY.
>
> I would also recommend making a dummy function so we can get rid of
> the conditional compilation in @verity_target below. For example:
>
> #ifdef CONFIG_SECURITY
> static int verity_finalize(struct dm_target *ti)
> {
> /* real implementation */
> }
> #else
> static int verity_finalize(struct dm_target *ti)
> {
> return 0;
> }
> #endif /* CONFIG_SECURITY */
>
>> static struct target_type verity_target = {
>> .name = "verity",
>> .features = DM_TARGET_SINGLETON | DM_TARGET_IMMUTABLE,
>> @@ -1596,6 +1666,9 @@ static struct target_type verity_target = {
>> .prepare_ioctl = verity_prepare_ioctl,
>> .iterate_devices = verity_iterate_devices,
>> .io_hints = verity_io_hints,
>> +#ifdef CONFIG_IPE_PROP_DM_VERITY
>> + .finalize = verity_finalize,
>> +#endif /* CONFIG_IPE_PROP_DM_VERITY */
>> };
>> module_dm(verity);
>
> If you create a dummy verity_finalize() function like above you can
> get rid of the #ifdef checks.
>
>> diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h
>> index 20b1bcf03474..6a5b8df5bafd 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_IPE_PROP_DM_VERITY
>> + u8 *root_digest_sig; /* digest signature of the root block */
>> +#endif /* CONFIG_IPE_PROP_DM_VERITY */
>> unsigned int salt_size;
>> sector_t data_start; /* data offset in 512-byte sectors */
>> sector_t hash_start; /* hash start in blocks */
>> @@ -56,6 +59,9 @@ struct dm_verity {
>> bool hash_failed:1; /* set if hash of any block failed */
>> bool use_bh_wq:1; /* try to verify in BH wq before normal work-queue */
>> unsigned int digest_size; /* digest size for the current hash algorithm */
>> +#ifdef CONFIG_IPE_PROP_DM_VERITY
>> + unsigned int sig_size; /* digest signature size */
>> +#endif /* CONFIG_IPE_PROP_DM_VERITY */
>> unsigned int ahash_reqsize;/* the size of temporary space for crypto */
>> enum verity_mode mode; /* mode for handling verification errors */
>> unsigned int corrupted_errs;/* Number of errors for corrupted blocks */
>
> See the previous comments about CONFIG_SECURITY.
>
>> 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 eaff8868766a..60b40b523d57 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -84,6 +84,8 @@ enum lsm_event {
>> };
>>
>> enum lsm_intgr_type {
>> + LSM_INTGR_DMV_SIG,
>> + LSM_INTGR_DMV_ROOTHASH,
>> __LSM_INTGR_MAX
>> };
>>
>> --
>> 2.44.0
>
> --
> paul-moore.com
next prev parent reply other threads:[~2024-03-20 17:56 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-16 3:35 [RFC PATCH v15 00/21] Integrity Policy Enforcement LSM (IPE) Fan Wu
2024-03-16 3:35 ` [RFC PATCH v15 01/21] security: add ipe lsm Fan Wu
2024-03-16 3:35 ` [RFC PATCH v15 02/21] ipe: add policy parser Fan Wu
2024-03-16 3:35 ` [RFC PATCH v15 03/21] ipe: add evaluation loop Fan Wu
2024-03-16 3:35 ` [RFC PATCH v15 04/21] ipe: add LSM hooks on execution and kernel read Fan Wu
2024-03-16 3:35 ` [RFC PATCH v15 05/21] initramfs|security: Add a security hook to do_populate_rootfs() Fan Wu
2024-03-18 0:29 ` Casey Schaufler
2024-03-18 1:58 ` Paul Moore
2024-03-16 3:35 ` [RFC PATCH v15 06/21] ipe: introduce 'boot_verified' as a trust provider Fan Wu
2024-03-16 3:35 ` [RFC PATCH v15 07/21] security: add new securityfs delete function Fan Wu
2024-03-16 3:35 ` [RFC PATCH v15 08/21] ipe: add userspace interface Fan Wu
2024-03-16 3:35 ` [RFC PATCH v15 09/21] uapi|audit|ipe: add ipe auditing support Fan Wu
2024-03-16 3:35 ` [RFC PATCH v15 10/21] ipe: add permissive toggle Fan Wu
2024-03-16 3:35 ` [RFC PATCH v15 11/21] block|security: add LSM blob to block_device Fan Wu
2024-03-16 3:35 ` [RFC PATCH v15 12/21] security: add security_bdev_setintegrity() hook Fan Wu
2024-03-19 23:00 ` [PATCH RFC " Paul Moore
2024-03-20 8:28 ` Jarkko Sakkinen
2024-03-20 8:31 ` Jarkko Sakkinen
2024-03-20 20:31 ` Fan Wu
2024-03-21 17:25 ` Jarkko Sakkinen
2024-03-16 3:35 ` [RFC PATCH v15 13/21] dm: add finalize hook to target_type Fan Wu
2024-03-16 3:35 ` [RFC PATCH v15 14/21] dm verity: consume root hash digest and signature data via LSM hook Fan Wu
2024-03-19 23:00 ` [PATCH RFC " Paul Moore
2024-03-20 2:19 ` Mike Snitzer
2024-03-20 17:23 ` Paul Moore
2024-03-20 18:49 ` Mike Snitzer
2024-03-20 17:56 ` Fan Wu [this message]
2024-03-16 3:35 ` [RFC PATCH v15 15/21] ipe: add support for dm-verity as a trust provider Fan Wu
2024-03-16 3:35 ` [RFC PATCH v15 16/21] security: add security_inode_setintegrity() hook Fan Wu
2024-03-19 23:00 ` [PATCH RFC " Paul Moore
2024-03-16 3:35 ` [RFC PATCH v15 17/21] fsverity: consume builtin signature via LSM hook Fan Wu
2024-03-18 5:29 ` Eric Biggers
2024-03-19 23:00 ` Paul Moore
2024-03-16 3:35 ` [RFC PATCH v15 18/21] ipe: enable support for fs-verity as a trust provider Fan Wu
2024-03-18 5:17 ` Eric Biggers
2024-03-18 8:08 ` Roberto Sassu
2024-03-18 20:58 ` Fan Wu
2024-03-18 20:40 ` Fan Wu
2024-03-16 3:35 ` [RFC PATCH v15 19/21] scripts: add boot policy generation program Fan Wu
2024-03-16 3:35 ` [RFC PATCH v15 20/21] ipe: kunit test for parser Fan Wu
2024-03-16 3:35 ` [RFC PATCH v15 21/21] documentation: add ipe documentation 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=2e76fd2f-f116-4aea-aa6d-4b3feef1c31e@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).