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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
> >> Signed-off-by: Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx>
> >> ---
> >> 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
