Received-SPF: pass (google.com: domain of linux-kernel+bounces-194961-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Thu, 30 May 2024 08:53:52 +0300
Message-Id: <D1MQVCEITGPS.2HU8JUF2MAYQ7@kernel.org>
Cc: <linux-doc@vger.kernel.org>, <linux-integrity@vger.kernel.org>,
 <linux-security-module@vger.kernel.org>, <fsverity@lists.linux.dev>,
 <linux-block@vger.kernel.org>, <dm-devel@lists.linux.dev>,
 <audit@vger.kernel.org>, <linux-kernel@vger.kernel.org>, "Deven Bowers"
 <deven.desai@linux.microsoft.com>
Subject: Re: [PATCH v19 13/20] ipe: add support for dm-verity as a trust
 provider
From: "Jarkko Sakkinen" <jarkko@kernel.org>
To: "Fan Wu" <wufan@linux.microsoft.com>, "Paul Moore"
 <paul@paul-moore.com>, <corbet@lwn.net>, <zohar@linux.ibm.com>,
 <jmorris@namei.org>, <serge@hallyn.com>, <tytso@mit.edu>,
 <ebiggers@kernel.org>, <axboe@kernel.dk>, <agk@redhat.com>,
 <snitzer@kernel.org>, <mpatocka@redhat.com>, <eparis@redhat.com>
References: <1716583609-21790-14-git-send-email-wufan@linux.microsoft.com>
 <2ecde610ca3f0cabcbb111e3432f2dd5@paul-moore.com>
 <67da2ff3-e0c4-4552-93dd-cf9cb04d0d78@linux.microsoft.com>
In-Reply-To: <67da2ff3-e0c4-4552-93dd-cf9cb04d0d78@linux.microsoft.com>

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 securit=
y/
> >>      & 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
> >=20
> > ...
> >=20
> >> 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 =3D 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 stru=
cture
> >> + *	  to free.
> >> + */
> >> +void ipe_bdev_free_security(struct block_device *bdev)
> >> +{
> >> +	struct ipe_bdev *blob =3D 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 =3D 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 e=
xistence
> >> + * of a validated signed dm-verity root hash into LSM blob.
> >> + *
> >> + * Return: %0 on success. If an error occurs, the function will retur=
n the
> >> + * -errno.
> >> + */
> >> +int ipe_bdev_setintegrity(struct block_device *bdev, enum lsm_integri=
ty_type type,
> >> +			  const void *value, size_t size)
> >> +{
> >> +	const struct dm_verity_digest *digest =3D NULL;
> >> +	struct ipe_bdev *blob =3D ipe_bdev(bdev);
> >> +	struct digest_info *info =3D NULL;
> >> +
> >> +	if (type =3D=3D LSM_INT_DMVERITY_ROOTHASH) {
> >> +		if (!value) {
> >> +			ipe_digest_free(blob->root_hash);
> >> +			blob->root_hash =3D NULL;
> >> +
> >> +			return 0;
> >> +		}
> >> +		digest =3D value;
> >> +
> >> +		info =3D kzalloc(sizeof(*info), GFP_KERNEL);
> >> +		if (!info)
> >> +			return -ENOMEM;
> >> +
> >> +		info->digest =3D kmemdup(digest->digest, digest->digest_len,
> >> +				       GFP_KERNEL);
> >> +		if (!info->digest)
> >> +			goto dmv_roothash_err;
> >> +
> >> +		info->alg =3D kstrdup(digest->alg, GFP_KERNEL);
> >> +		if (!info->alg)
> >> +			goto dmv_roothash_err;
> >> +
> >> +		info->digest_len =3D digest->digest_len;
> >> +
> >> +		if (blob->root_hash)
> >> +			ipe_digest_free(blob->root_hash);
> >=20
> > 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.
> >=20
> > Likely harmless and doubtful to have any noticable performance impact,
> > but I wanted to mention it just in case ...
> >=20
>
> Yes directly call ipe_digest_free() should be enough.
>
> Also this new free is introduced because the mapped device with an=20
> existing dm-verity target can be suspended and associated with a new=20
> dm-verity target. In this case, the root hash associated with the=20
> security blob will be stale and needs to be freed before setting the new=
=20
> data.
>
> -Fan
>
> >> +		blob->root_hash =3D 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 =3D=3D LSM_INT_DMVERITY_SIG_VALID) {
> >> +		ipe_set_dmverity_signature(blob, value, size);
> >> +
> >> +		return 0;
> >> +	}
> >> +
> >> +	return -EINVAL;
> >> +}
> >> +#endif /* CONFIG_IPE_PROP_DM_VERITY */
> >=20
> > --
> > paul-moore.com


BR, Jarkko