Received-SPF: pass (google.com: domain of linux-kernel+bounces-159119-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <6cf278b3-32f2-4665-be8d-ea6605f4318b@linux.microsoft.com>
Date: Thu, 25 Apr 2024 13:23:57 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v17 13/21] dm verity: consume root hash digest and expose
 signature data via LSM hook
To: Eric Biggers <ebiggers@kernel.org>
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,
 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>
References: <1712969764-31039-1-git-send-email-wufan@linux.microsoft.com>
 <1712969764-31039-14-git-send-email-wufan@linux.microsoft.com>
 <20240425035647.GC1401@sol.localdomain>
Content-Language: en-CA
From: Fan Wu <wufan@linux.microsoft.com>
In-Reply-To: <20240425035647.GC1401@sol.localdomain>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 4/24/2024 8:56 PM, Eric Biggers wrote:
> On Fri, Apr 12, 2024 at 05:55:56PM -0700, Fan Wu wrote:
>> dm verity: consume root hash digest and expose signature data via LSM hook
> 
> As in the fsverity patch, nothing is being "consumed" here.  This patch adds a
> supplier, not a consumer.  I think you mean something like: expose root digest
> and signature to LSMs.
> 
Thanks for the suggestion.

>> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
>> index bb5da66da4c1..fbb83c6fd99c 100644
>> --- a/drivers/md/dm-verity-target.c
>> +++ b/drivers/md/dm-verity-target.c
>> @@ -22,6 +22,8 @@
>>   #include <linux/scatterlist.h>
>>   #include <linux/string.h>
>>   #include <linux/jump_label.h>
>> +#include <linux/security.h>
>> +#include <linux/dm-verity.h>
>>   
>>   #define DM_MSG_PREFIX			"verity"
>>   
>> @@ -1017,6 +1019,38 @@ static void verity_io_hints(struct dm_target *ti, struct queue_limits *limits)
>>   	blk_limits_io_min(limits, limits->logical_block_size);
>>   }
>>   
>> +#ifdef CONFIG_SECURITY
>> +
>> +static int verity_init_sig(struct dm_verity *v, const void *sig,
>> +			   size_t sig_size)
>> +{
>> +	v->sig_size = sig_size;
>> +	v->root_digest_sig = kmemdup(sig, v->sig_size, GFP_KERNEL);
>> +	if (!v->root_digest)
>> +		return -ENOMEM;
> 
> root_digest_sig, not root_digest
> 
Thanks for pointing out!

>> +#ifdef CONFIG_SECURITY
>> +
>> +static int verity_finalize(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 = v->alg_name;
>> +
>> +	r = security_bdev_setintegrity(bdev, LSM_INT_DMVERITY_ROOTHASH, &root_digest,
>> +				       sizeof(root_digest));
>> +	if (r)
>> +		return r;
>> +
>> +	r = security_bdev_setintegrity(bdev,
>> +				       LSM_INT_DMVERITY_SIG_VALID,
>> +				       v->root_digest_sig,
>> +				       v->sig_size);
> 
> The signature is only checked if CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG=y, whereas
> this code is built whenever CONFIG_SECURITY=y.
> 
> So this seems like the same issue that has turned up elsewhere in the IPE
> patchset, where IPE is (apparently) happy with any signature, even one that
> hasn't been checked...
> 

Yes I do agree the second hook call should better depend on 
CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG=y.

However, the current implementation does not happy with any signature.

In case of CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG=y, any signature 
provided to dm-verity will be checked against the configured keyring, 
the hook call won't be reached if the check failed. In case of no 
signature is provided and !DM_VERITY_IS_SIG_FORCE_ENABLED(), the hook 
will be called with signature value NULL.

In case of CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG=n, signature won't be 
accepted by dm-verity. In addition, the whole support of dm-verity will 
be disabled for IPE because CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG=n.

>> diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h
>> index 20b1bcf03474..89e862f0cdf6 100644
>> --- a/drivers/md/dm-verity.h
>> +++ b/drivers/md/dm-verity.h
>> @@ -43,6 +43,9 @@ struct dm_verity {
>>   	u8 *root_digest;	/* digest of the root block */
>>   	u8 *salt;		/* salt: its size is salt_size */
>>   	u8 *zero_digest;	/* digest for a zero block */
>> +#ifdef CONFIG_SECURITY
>> +	u8 *root_digest_sig;	/* digest signature of the root block */
>> +#endif /* CONFIG_SECURITY */
> 
> No, it's not a signature of the root block, at least not directly.  It's a
> signature of the root digest (the digest of the root block).
> 
>> diff --git a/include/linux/dm-verity.h b/include/linux/dm-verity.h
>> new file mode 100644
>> index 000000000000..a799a8043d85
>> --- /dev/null
>> +++ b/include/linux/dm-verity.h
>> @@ -0,0 +1,12 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#ifndef _LINUX_DM_VERITY_H
>> +#define _LINUX_DM_VERITY_H
>> +
>> +struct dm_verity_digest {
>> +	const char *alg;
>> +	const u8 *digest;
>> +	size_t digest_len;
>> +};
>> +
>> +#endif /* _LINUX_DM_VERITY_H */
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index ac0985641611..9e46b13a356c 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -84,7 +84,8 @@ enum lsm_event {
>>   };
>>   
>>   enum lsm_integrity_type {
>> -	__LSM_INT_MAX
>> +	LSM_INT_DMVERITY_SIG_VALID,
>> +	LSM_INT_DMVERITY_ROOTHASH,
>>   };
> 
> Shouldn't struct dm_verity_digest be defined next to LSM_INT_DMVERITY_ROOTHASH?
> It's the struct that's associated with it.
> 
> It seems weird to create a brand new header <linux/dm-verity.h> that just
> contains this one LSM related definition, when there's already a header for the
> LSM definitions that even includes the related value LSM_INT_DMVERITY_ROOTHASH.
> 
> - Eric

Yes they can just be in the same header. Thanks for the suggestion.

-Fan