Received-SPF: pass (google.com: domain of linux-kernel+bounces-197467-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 31 May 2024 20:48:42 +0000
From: Eric Biggers <ebiggers@kernel.org>
To: Fan Wu <wufan@linux.microsoft.com>
Cc: corbet@lwn.net, zohar@linux.ibm.com, jmorris@namei.org,
	serge@hallyn.com, tytso@mit.edu, axboe@kernel.dk, agk@redhat.com,
	snitzer@kernel.org, mpatocka@redhat.com, eparis@redhat.com,
	paul@paul-moore.com, 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 11/20] block,lsm: add LSM blob and new LSM hooks for
 block device
Message-ID: <20240531204842.GA2838215@google.com>
References: <1716583609-21790-1-git-send-email-wufan@linux.microsoft.com>
 <1716583609-21790-12-git-send-email-wufan@linux.microsoft.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1716583609-21790-12-git-send-email-wufan@linux.microsoft.com>

On Fri, May 24, 2024 at 01:46:40PM -0700, Fan Wu wrote:
> From: Deven Bowers <deven.desai@linux.microsoft.com>
> 
> Some block devices have valuable security properties that is only
> accessible during the creation time.
>
> For example, when creating a dm-verity block device, the dm-verity's
> roothash and roothash signature, which are extreme important security
> metadata, are passed to the kernel. However, the roothash will be saved
> privately in dm-verity, which prevents the security subsystem to easily
> access that information. Worse, in the current implementation the
> roothash signature will be discarded after the verification, making it
> impossible to utilize the roothash signature by the security subsystem.

This patch seems to be assuming that creating the block device == setting up
dm-verity.  That's not how it actually works.  The way that device-mapper works
is that first a device-mapper device is created, and then targets are loaded
into it.  The targets can be changed later, any number of times.

So, while the creation of the block device is when the LSM blob is allocated,
it's not when the actual contents of it are initialized.  And its contents can
vary over the lifetime of the block device, including changing from something
the LSM "trusts" to something it doesn't "trust".

I'm not sure if this is "just" a documentation issue or if there are bugs
resulting from not handling changes properly.  The code itself *looks* correct,
but seeing it's not clear how much this has been considered and that getting
this wrong would allow the LSM checks to be bypassed, I thought I'd draw
attention to it.  This is really something that ought to be called out
explicitly in comments, for example.

> For example, LSM can use the new LSM blob to save the roothash signature of a
> dm-verity, and LSM can make access decision based on the data inside the
> signature, like the signer certificate.

This isn't what IPE actually does, though.  So this doesn't seem like a
particularly useful example in this context.

> For example, for dm-verity, LSMs can use this hook to save
> the roothash signature of a dm-verity into the security blob,
> and LSMs can make access decisions based on the data inside
> the signature, like the signer certificate.

Likewise.

> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index 781c4500491b..eaa28f366d98 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -71,6 +71,9 @@ struct block_device {
>  
>  	struct partition_meta_info *bd_meta_info;
>  	int			bd_writers;
> +#ifdef CONFIG_SECURITY
> +	void			*security;
> +#endif

All the other fields in struct block_device are prefixed with "bd_", so please
use the same pattern for this new field (bd_security).

> diff --git a/security/security.c b/security/security.c
> index b419166979da..743652e5e893 100644
> --- a/security/security.c
> +++ b/security/security.c
[...]
> +/**
> + * security_bdev_setintegrity() - Set the device's integrity data
> + * @bdev: block device
> + * @type: type of integrity, e.g. hash digest, signature, etc
> + * @value: the integrity value
> + * @size: size of the integrity value
> + *
> + * Register a verified integrity measurement of a bdev with LSMs.
> + * LSMs should free the previously saved data if @value is NULL.
> + *
> + * Return: Returns 0 on success, negative values on failure.
> + */
> +int security_bdev_setintegrity(struct block_device *bdev,
> +			       enum lsm_integrity_type type, const void *value,
> +			       size_t size)
> +{
> +	return call_int_hook(bdev_setintegrity, bdev, type, value, size);
> +}
> +EXPORT_SYMBOL(security_bdev_setintegrity);

This might be a good place to explicitly document that the block device's
integrity properties may change over the lifetime of the block device and that
LSMs *must* (not "should") handle all possible types of updates, including
updates from a non-NULL value of a property to a NULL value.

- Eric