Re: [PATCH v19 12/20] dm verity: expose root hash digest and signature data to LSMs

[Eric Biggers <ebiggers@kernel.org>]

On Fri, May 24, 2024 at 01:46:41PM -0700, Fan Wu wrote:
> +#ifdef CONFIG_SECURITY
> +
> +#ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG
> +
> +static int verity_security_set_signature(struct block_device *bdev,
> +					 struct dm_verity *v)
> +{
> +	return security_bdev_setintegrity(bdev,
> +					  LSM_INT_DMVERITY_SIG_VALID,
> +					  v->root_digest_sig,
> +					  v->sig_size);
> +}
> +
> +#else
> +
> +static inline int verity_security_set_signature(struct block_device *bdev,
> +						struct dm_verity *v)
> +{
> +	return 0;
> +}
> +
> +#endif /* CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG */
> +
> +/*
> + * Expose verity target's root hash and signature data to LSMs before resume.
> + *
> + * Returns 0 on success, or -ENOMEM if the system is out of memory.
> + */
> +static int verity_preresume(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 = crypto_ahash_alg_name(v->tfm);
> +
> +	r = security_bdev_setintegrity(bdev, LSM_INT_DMVERITY_ROOTHASH, &root_digest,
> +				       sizeof(root_digest));
> +	if (r)
> +		return r;
> +
> +	r =  verity_security_set_signature(bdev, v);
> +	if (r)
> +		goto bad;
> +
> +	return 0;
> +
> +bad:
> +
> +	security_bdev_setintegrity(bdev, LSM_INT_DMVERITY_ROOTHASH, NULL, 0);
> +
> +	return r;
> +}
> +
> +#endif /* CONFIG_SECURITY */
> +
>  static struct target_type verity_target = {
>  	.name		= "verity",
>  	.features	= DM_TARGET_SINGLETON | DM_TARGET_IMMUTABLE,

Due to the possibility of table reloads, it looks like the security of this
scheme is dependent on (a) DM_TARGET_SINGLETON, (b) DM_TARGET_IMMUTABLE, *and*
(c) sending LSM_INT_DMVERITY_ROOTHASH and LSM_INT_DMVERITY_SIG_VALID to the
LSM(s) even when there is no signature.  Notably, this differs from the
similar-looking code in fsverity where updates are not possible and
LSM_INT_FSVERITY_BUILTINSIG_VALID is not sent when there's no signature.

Given the subtleties here and the fact that getting any of these things wrong
would allow the LSM checks to be bypassed, it would really be worth leaving a
comment that explicitly documents why this is secure.  And maybe also a 
/* Note: singleton and immutable are depended on by the LSM hooks */ just above
the 'DM_TARGET_SINGLETON | DM_TARGET_IMMUTABLE' in case someone tries to remove
those.  I see they were added only recently, which was a breaking UAPI change,
so I worry about people trying to revert it.

- Eric