| Message ID | 1710560151-28904-15-git-send-email-wufan@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Integrity Policy Enforcement LSM (IPE) | expand |
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. > + 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? > +} > + > +#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
On Tue, Mar 19 2024 at 7:00P -0400, Paul Moore <paul@paul-moore.com> 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. > > > + 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? > > > +} > > + > > +#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. Think it is better to leave it as-is, to avoid calling the .finalize hook if it isn't actually needed. Mike
On Tue, Mar 19, 2024 at 10:19 PM Mike Snitzer <snitzer@kernel.org> wrote: > On Tue, Mar 19 2024 at 7:00P -0400, > Paul Moore <paul@paul-moore.com> 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. > > > > > + 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? > > > > > +} > > > + > > > +#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. > > Think it is better to leave it as-is, to avoid calling the .finalize > hook if it isn't actually needed. Fair enough, my personal preference is to minimize Kconfig conditional code flow changes such as this, but I understand your point of view and device-mapper is your code after all. I believe the other issues still need to be addressed, but other than that are you generally okay with the new "finalize" hook approach?
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
On Wed, Mar 20 2024 at 1:23P -0400, Paul Moore <paul@paul-moore.com> wrote: > On Tue, Mar 19, 2024 at 10:19 PM Mike Snitzer <snitzer@kernel.org> wrote: > > On Tue, Mar 19 2024 at 7:00P -0400, > > Paul Moore <paul@paul-moore.com> 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. > > > > > > > + 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? > > > > > > > +} > > > > + > > > > +#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. > > > > Think it is better to leave it as-is, to avoid calling the .finalize > > hook if it isn't actually needed. > > Fair enough, my personal preference is to minimize Kconfig conditional > code flow changes such as this, but I understand your point of view > and device-mapper is your code after all. > > I believe the other issues still need to be addressed, Yes. > but other than that are you generally okay with the new "finalize" > hook approach? I am, not seeing how we could avoid it. Thanks, Mike
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; + + 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 */ + 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); +} + +#endif /* CONFIG_IPE_PROP_DM_VERITY */ + 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); 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 */ 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 };