Re: [PATCH v19 13/20] ipe: add support for dm-verity as a trust provider

["Jarkko Sakkinen" <jarkko@kernel.org>]

On Thu May 30, 2024 at 6:58 AM EEST, Fan Wu wrote:
>
>
> On 5/29/2024 6:44 PM, Paul Moore wrote:
> > On May 24, 2024 Fan Wu <wufan@linux.microsoft.com> wrote:
> >>
> >> Allows author of IPE policy to indicate trust for a singular dm-verity
> >> volume, identified by roothash, through "dmverity_roothash" and all
> >> signed and validated dm-verity volumes, through "dmverity_signature".
> >>
> >> 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]
> >>
> >> 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.
> >>
> >> v9:
> >>    + Adapt to the new parser
> >>
> >> v10:
> >>    + Select the Kconfig when all dependencies are enabled
> >>
> >> v11:
> >>    + No changes
> >>
> >> v12:
> >>    + Refactor to use struct digest_info* instead of void*
> >>    + Correct audit format
> >>
> >> v13:
> >>    + Remove the CONFIG_IPE_PROP_DM_VERITY dependency inside the parser
> >>      to make the policy grammar independent of the kernel config.
> >>
> >> v14:
> >>    + No changes
> >>
> >> v15:
> >>    + Fix one grammar issue in KCONFIG
> >>    + Switch to use security_bdev_setintegrity() hook
> >>
> >> v16:
> >>    + Refactor for enum integrity type
> >>
> >> v17:
> >>    + Add years to license header
> >>    + Fix code and documentation style issues
> >>    + Return -EINVAL in ipe_bdev_setintegrity when passed type is not
> >>      supported
> >>    + Use new enum name LSM_INT_DMVERITY_SIG_VALID
> >>
> >> v18:
> >>    + Add Kconfig IPE_PROP_DM_VERITY_SIGNATURE and make both DM_VERITY
> >>      config auto-selected
> >>
> >> v19:
> >>    + No changes
> >> ---
> >>   security/ipe/Kconfig         |  27 ++++++++
> >>   security/ipe/Makefile        |   1 +
> >>   security/ipe/audit.c         |  29 ++++++++-
> >>   security/ipe/digest.c        | 118 +++++++++++++++++++++++++++++++++++
> >>   security/ipe/digest.h        |  26 ++++++++
> >>   security/ipe/eval.c          |  93 ++++++++++++++++++++++++++-
> >>   security/ipe/eval.h          |  12 ++++
> >>   security/ipe/hooks.c         |  93 +++++++++++++++++++++++++++
> >>   security/ipe/hooks.h         |   8 +++
> >>   security/ipe/ipe.c           |  15 +++++
> >>   security/ipe/ipe.h           |   4 ++
> >>   security/ipe/policy.h        |   3 +
> >>   security/ipe/policy_parser.c |  24 ++++++-
> >>   13 files changed, 449 insertions(+), 4 deletions(-)
> >>   create mode 100644 security/ipe/digest.c
> >>   create mode 100644 security/ipe/digest.h
> > 
> > ...
> > 
> >> diff --git a/security/ipe/hooks.c b/security/ipe/hooks.c
> >> index b68719bf44fb..51f1e63c295c 100644
> >> --- a/security/ipe/hooks.c
> >> +++ b/security/ipe/hooks.c
> >> @@ -191,3 +193,94 @@ void ipe_unpack_initramfs(void)
> >>   {
> >>   	ipe_sb(current->fs->root.mnt->mnt_sb)->initramfs = true;
> >>   }
> >> +
> >> +#ifdef CONFIG_IPE_PROP_DM_VERITY
> >> +/**
> >> + * ipe_bdev_free_security() - Free IPE's LSM blob of block_devices.
> >> + * @bdev: Supplies a pointer to a block_device that contains the structure
> >> + *	  to free.
> >> + */
> >> +void ipe_bdev_free_security(struct block_device *bdev)
> >> +{
> >> +	struct ipe_bdev *blob = ipe_bdev(bdev);
> >> +
> >> +	ipe_digest_free(blob->root_hash);
> >> +}
> >> +
> >> +#ifdef CONFIG_IPE_PROP_DM_VERITY_SIGNATURE
> >> +static void ipe_set_dmverity_signature(struct ipe_bdev *blob,
> >> +				       const void *value,
> >> +				       size_t size)
> >> +{
> >> +	blob->dm_verity_signed = size > 0 && value;
> >> +}
> >> +#else
> >> +static inline void ipe_set_dmverity_signature(struct ipe_bdev *blob,
> >> +					      const void *value,
> >> +					      size_t size)
> >> +{
> >> +}
> >> +#endif /* CONFIG_IPE_PROP_DM_VERITY_SIGNATURE */
> >> +
> >> +/**
> >> + * ipe_bdev_setintegrity() - Save integrity data from a bdev to IPE's LSM blob.
> >> + * @bdev: Supplies a pointer to a block_device that contains the LSM blob.
> >> + * @type: Supplies the integrity type.
> >> + * @value: Supplies the value to store.
> >> + * @size: The size of @value.
> >> + *
> >> + * This hook is currently used to save dm-verity's root hash or the existence
> >> + * of a validated signed dm-verity root hash into LSM blob.
> >> + *
> >> + * Return: %0 on success. If an error occurs, the function will return the
> >> + * -errno.
> >> + */
> >> +int ipe_bdev_setintegrity(struct block_device *bdev, enum lsm_integrity_type type,
> >> +			  const void *value, size_t size)
> >> +{
> >> +	const struct dm_verity_digest *digest = NULL;
> >> +	struct ipe_bdev *blob = ipe_bdev(bdev);
> >> +	struct digest_info *info = NULL;
> >> +
> >> +	if (type == LSM_INT_DMVERITY_ROOTHASH) {
> >> +		if (!value) {
> >> +			ipe_digest_free(blob->root_hash);
> >> +			blob->root_hash = NULL;
> >> +
> >> +			return 0;
> >> +		}
> >> +		digest = value;
> >> +
> >> +		info = kzalloc(sizeof(*info), GFP_KERNEL);
> >> +		if (!info)
> >> +			return -ENOMEM;
> >> +
> >> +		info->digest = kmemdup(digest->digest, digest->digest_len,
> >> +				       GFP_KERNEL);
> >> +		if (!info->digest)
> >> +			goto dmv_roothash_err;
> >> +
> >> +		info->alg = kstrdup(digest->alg, GFP_KERNEL);
> >> +		if (!info->alg)
> >> +			goto dmv_roothash_err;
> >> +
> >> +		info->digest_len = digest->digest_len;
> >> +
> >> +		if (blob->root_hash)
> >> +			ipe_digest_free(blob->root_hash);
> > 
> > The above if/free looks like a new addition from v18 and I'm not quite
> > sure why the `blob->root_hash` NULL check is necessary as
> > ipe_digest_free() does a IS_ERR_OR_NULL() check right at the top.
> > 
> > Likely harmless and doubtful to have any noticable performance impact,
> > but I wanted to mention it just in case ...
> > 
>
> Yes directly call ipe_digest_free() should be enough.
>
> Also this new free is introduced because the mapped device with an 
> existing dm-verity target can be suspended and associated with a new 
> dm-verity target. In this case, the root hash associated with the 
> security blob will be stale and needs to be freed before setting the new 
> data.
>
> -Fan
>
> >> +		blob->root_hash = info;
> >> +
> >> +		return 0;
> >> +dmv_roothash_err:

Just a nitpick but 9/10 'err' is a prefix...

Also now this patch set uses 'err'' ambiguously given the use
as name of the variable to store a return code. Similar naming
pattern would do miracles.

> >> +		ipe_digest_free(info);
> >> +
> >> +		return -ENOMEM;
> >> +	} else if (type == LSM_INT_DMVERITY_SIG_VALID) {
> >> +		ipe_set_dmverity_signature(blob, value, size);
> >> +
> >> +		return 0;
> >> +	}
> >> +
> >> +	return -EINVAL;
> >> +}
> >> +#endif /* CONFIG_IPE_PROP_DM_VERITY */
> > 
> > --
> > paul-moore.com


BR, Jarkko