diff mbox series

[RFC,v15,14/21] dm verity: consume root hash digest and signature data via LSM hook

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

Commit Message

Fan Wu March 16, 2024, 3:35 a.m. UTC
From: Deven Bowers <deven.desai@linux.microsoft.com>

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

Comments

Paul Moore March 19, 2024, 11 p.m. UTC | #1
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
Mike Snitzer March 20, 2024, 2:19 a.m. UTC | #2
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
Paul Moore March 20, 2024, 5:23 p.m. UTC | #3
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?
Fan Wu March 20, 2024, 5:56 p.m. UTC | #4
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
Mike Snitzer March 20, 2024, 6:49 p.m. UTC | #5
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 mbox series

Patch

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
 };