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

[Fan Wu <wufan@linux.microsoft.com>]

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:
>> +		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