dm verity: fallback to platform keyring also if key in trusted keyring is rejected

Commit Message

Luca Boccassi Sept. 22, 2024, 4:17 p.m. UTC

From: Luca Boccassi <[e-mail]>

If enabled, we fallback to the platform keyring if the trusted keyring doesn't have
the key used to sign the roothash. But if pkcs7_verify() rejects the key for other
reasons, such as usage restrictions, we do not fallback. Do so.

Follow-up for 6fce1f40e95182ebbfe1ee3096b8fc0b37903269

Suggested-by: Serge Hallyn <[e-mail]>
Signed-off-by: Luca Boccassi <[e-mail]>
---
 drivers/md/dm-verity-verify-sig.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Mikulas Patocka Sept. 23, 2024, 2:04 p.m. UTC | #1

On Sun, 22 Sep 2024, [e-mail] wrote:

> From: Luca Boccassi <[e-mail]>
> 
> If enabled, we fallback to the platform keyring if the trusted keyring doesn't have
> the key used to sign the roothash. But if pkcs7_verify() rejects the key for other
> reasons, such as usage restrictions, we do not fallback. Do so.
> 
> Follow-up for 6fce1f40e95182ebbfe1ee3096b8fc0b37903269
> 
> Suggested-by: Serge Hallyn <[e-mail]>
> Signed-off-by: Luca Boccassi <[e-mail]>

Hi

I'm not an expert in keyrings.

I added keyring maintainers to the CC. Please review this patch and 
Ack/Nack it.

Mikulas

> ---
>  drivers/md/dm-verity-verify-sig.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
> index d351d7d39c60..a9e2c6c0a33c 100644
> --- a/drivers/md/dm-verity-verify-sig.c
> +++ b/drivers/md/dm-verity-verify-sig.c
> @@ -127,7 +127,7 @@ int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
>  #endif
>  				VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
>  #ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_PLATFORM_KEYRING
> -	if (ret == -ENOKEY)
> +	if (ret == -ENOKEY || ret == -EKEYREJECTED)
>  		ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
>  					sig_len,
>  					VERIFY_USE_PLATFORM_KEYRING,
> -- 
> 2.39.5
>

Jarkko Sakkinen Sept. 24, 2024, 3:54 p.m. UTC | #2

On Mon Sep 23, 2024 at 5:04 PM EEST, Mikulas Patocka wrote:
>
>
> On Sun, 22 Sep 2024, [e-mail] wrote:
>
> > From: Luca Boccassi <[e-mail]>
> > 
> > If enabled, we fallback to the platform keyring if the trusted keyring doesn't have
> > the key used to sign the roothash. But if pkcs7_verify() rejects the key for other
> > reasons, such as usage restrictions, we do not fallback. Do so.
> > 
> > Follow-up for 6fce1f40e95182ebbfe1ee3096b8fc0b37903269
> > 
> > Suggested-by: Serge Hallyn <[e-mail]>
> > Signed-off-by: Luca Boccassi <[e-mail]>
>
> Hi
>
> I'm not an expert in keyrings.
>
> I added keyring maintainers to the CC. Please review this patch and 
> Ack/Nack it.
>
> Mikulas
>
> > ---
> >  drivers/md/dm-verity-verify-sig.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
> > index d351d7d39c60..a9e2c6c0a33c 100644
> > --- a/drivers/md/dm-verity-verify-sig.c
> > +++ b/drivers/md/dm-verity-verify-sig.c
> > @@ -127,7 +127,7 @@ int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
> >  #endif
> >  				VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
> >  #ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_PLATFORM_KEYRING
> > -	if (ret == -ENOKEY)
> > +	if (ret == -ENOKEY || ret == -EKEYREJECTED)
> >  		ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
> >  					sig_len,
> >  					VERIFY_USE_PLATFORM_KEYRING,
> > -- 
> > 2.39.5
> > 

I know nothing about dm-verity. What does it even do?

BR, Jarkko

Mikulas Patocka Sept. 24, 2024, 6:27 p.m. UTC | #3

On Tue, 24 Sep 2024, Jarkko Sakkinen wrote:

> On Mon Sep 23, 2024 at 5:04 PM EEST, Mikulas Patocka wrote:
> >
> >
> > On Sun, 22 Sep 2024, [e-mail] wrote:
> >
> > > From: Luca Boccassi <[e-mail]>
> > > 
> > > If enabled, we fallback to the platform keyring if the trusted keyring doesn't have
> > > the key used to sign the roothash. But if pkcs7_verify() rejects the key for other
> > > reasons, such as usage restrictions, we do not fallback. Do so.
> > > 
> > > Follow-up for 6fce1f40e95182ebbfe1ee3096b8fc0b37903269
> > > 
> > > Suggested-by: Serge Hallyn <[e-mail]>
> > > Signed-off-by: Luca Boccassi <[e-mail]>
> >
> > Hi
> >
> > I'm not an expert in keyrings.
> >
> > I added keyring maintainers to the CC. Please review this patch and 
> > Ack/Nack it.
> >
> > Mikulas
> >
> > > ---
> > >  drivers/md/dm-verity-verify-sig.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
> > > index d351d7d39c60..a9e2c6c0a33c 100644
> > > --- a/drivers/md/dm-verity-verify-sig.c
> > > +++ b/drivers/md/dm-verity-verify-sig.c
> > > @@ -127,7 +127,7 @@ int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
> > >  #endif
> > >  				VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
> > >  #ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_PLATFORM_KEYRING
> > > -	if (ret == -ENOKEY)
> > > +	if (ret == -ENOKEY || ret == -EKEYREJECTED)
> > >  		ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
> > >  					sig_len,
> > >  					VERIFY_USE_PLATFORM_KEYRING,
> > > -- 
> > > 2.39.5
> > > 
> 
> I know nothing about dm-verity. What does it even do?
> 
> BR, Jarkko

dm-verity provides a read-only device with integrity checking. dm-verity 
stores hash for every block on the block device and checks the hash when 
reading the block. If the hash doesn't match, it can do one of these 
actions (depending on configuration):
- return I/O error
- try to correct the data using forward error correction
- log the mismatch and do nothing
- restart the machine
- call panic()

dm-verity is mostly used for the immutable system partition on Android 
phones. For more info, see 
Documentation/admin-guide/device-mapper/verity.rst

The above patch changes the way that the signature of the root hash is 
verified. I have no clue whether the patch can or can't subvert system 
security, that's why I'd like to have some more reviews of the patch 
before accepting it.

Mikulas

Jarkko Sakkinen Sept. 24, 2024, 9:36 p.m. UTC | #4

On Tue Sep 24, 2024 at 9:27 PM EEST, Mikulas Patocka wrote:
>
>
> On Tue, 24 Sep 2024, Jarkko Sakkinen wrote:
>
> > On Mon Sep 23, 2024 at 5:04 PM EEST, Mikulas Patocka wrote:
> > >
> > >
> > > On Sun, 22 Sep 2024, [e-mail] wrote:
> > >
> > > > From: Luca Boccassi <[e-mail]>
> > > > 
> > > > If enabled, we fallback to the platform keyring if the trusted keyring doesn't have
> > > > the key used to sign the roothash. But if pkcs7_verify() rejects the key for other
> > > > reasons, such as usage restrictions, we do not fallback. Do so.
> > > > 
> > > > Follow-up for 6fce1f40e95182ebbfe1ee3096b8fc0b37903269
> > > > 
> > > > Suggested-by: Serge Hallyn <[e-mail]>
> > > > Signed-off-by: Luca Boccassi <[e-mail]>
> > >
> > > Hi
> > >
> > > I'm not an expert in keyrings.
> > >
> > > I added keyring maintainers to the CC. Please review this patch and 
> > > Ack/Nack it.
> > >
> > > Mikulas
> > >
> > > > ---
> > > >  drivers/md/dm-verity-verify-sig.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
> > > > index d351d7d39c60..a9e2c6c0a33c 100644
> > > > --- a/drivers/md/dm-verity-verify-sig.c
> > > > +++ b/drivers/md/dm-verity-verify-sig.c
> > > > @@ -127,7 +127,7 @@ int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
> > > >  #endif
> > > >  				VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
> > > >  #ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_PLATFORM_KEYRING
> > > > -	if (ret == -ENOKEY)
> > > > +	if (ret == -ENOKEY || ret == -EKEYREJECTED)
> > > >  		ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
> > > >  					sig_len,
> > > >  					VERIFY_USE_PLATFORM_KEYRING,
> > > > -- 
> > > > 2.39.5
> > > > 
> > 
> > I know nothing about dm-verity. What does it even do?
> > 
> > BR, Jarkko
>
> dm-verity provides a read-only device with integrity checking. dm-verity 
> stores hash for every block on the block device and checks the hash when 
> reading the block. If the hash doesn't match, it can do one of these 
> actions (depending on configuration):
> - return I/O error
> - try to correct the data using forward error correction
> - log the mismatch and do nothing
> - restart the machine
> - call panic()
>
> dm-verity is mostly used for the immutable system partition on Android 
> phones. For more info, see 
> Documentation/admin-guide/device-mapper/verity.rst
>
> The above patch changes the way that the signature of the root hash is 
> verified. I have no clue whether the patch can or can't subvert system 
> security, that's why I'd like to have some more reviews of the patch 
> before accepting it.

I guess someone who knows all this already should review it.

Doesn't dm-verity have a maintainer?

BR, Jarkko

Eric Biggers Sept. 24, 2024, 9:59 p.m. UTC | #5

On Wed, Sep 25, 2024 at 12:36:01AM +0300, Jarkko Sakkinen wrote:
> On Tue Sep 24, 2024 at 9:27 PM EEST, Mikulas Patocka wrote:
> >
> >
> > On Tue, 24 Sep 2024, Jarkko Sakkinen wrote:
> >
> > > On Mon Sep 23, 2024 at 5:04 PM EEST, Mikulas Patocka wrote:
> > > >
> > > >
> > > > On Sun, 22 Sep 2024, [e-mail] wrote:
> > > >
> > > > > From: Luca Boccassi <[e-mail]>
> > > > > 
> > > > > If enabled, we fallback to the platform keyring if the trusted keyring doesn't have
> > > > > the key used to sign the roothash. But if pkcs7_verify() rejects the key for other
> > > > > reasons, such as usage restrictions, we do not fallback. Do so.
> > > > > 
> > > > > Follow-up for 6fce1f40e95182ebbfe1ee3096b8fc0b37903269
> > > > > 
> > > > > Suggested-by: Serge Hallyn <[e-mail]>
> > > > > Signed-off-by: Luca Boccassi <[e-mail]>
> > > >
> > > > Hi
> > > >
> > > > I'm not an expert in keyrings.
> > > >
> > > > I added keyring maintainers to the CC. Please review this patch and 
> > > > Ack/Nack it.
> > > >
> > > > Mikulas
> > > >
> > > > > ---
> > > > >  drivers/md/dm-verity-verify-sig.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
> > > > > index d351d7d39c60..a9e2c6c0a33c 100644
> > > > > --- a/drivers/md/dm-verity-verify-sig.c
> > > > > +++ b/drivers/md/dm-verity-verify-sig.c
> > > > > @@ -127,7 +127,7 @@ int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
> > > > >  #endif
> > > > >  				VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
> > > > >  #ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_PLATFORM_KEYRING
> > > > > -	if (ret == -ENOKEY)
> > > > > +	if (ret == -ENOKEY || ret == -EKEYREJECTED)
> > > > >  		ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
> > > > >  					sig_len,
> > > > >  					VERIFY_USE_PLATFORM_KEYRING,
> > > > > -- 
> > > > > 2.39.5
> > > > > 
> > > 
> > > I know nothing about dm-verity. What does it even do?
> > > 
> > > BR, Jarkko
> >
> > dm-verity provides a read-only device with integrity checking. dm-verity 
> > stores hash for every block on the block device and checks the hash when 
> > reading the block. If the hash doesn't match, it can do one of these 
> > actions (depending on configuration):
> > - return I/O error
> > - try to correct the data using forward error correction
> > - log the mismatch and do nothing
> > - restart the machine
> > - call panic()
> >
> > dm-verity is mostly used for the immutable system partition on Android 
> > phones. For more info, see 
> > Documentation/admin-guide/device-mapper/verity.rst
> >
> > The above patch changes the way that the signature of the root hash is 
> > verified. I have no clue whether the patch can or can't subvert system 
> > security, that's why I'd like to have some more reviews of the patch 
> > before accepting it.
> 
> I guess someone who knows all this already should review it.
> 
> Doesn't dm-verity have a maintainer?
> 

This patch only affects dm-verity's in-kernel signature verification support,
which has only been present since Linux v5.4 and is not used by Android or
Chrome OS.  The whole feature seems weird to me, and it is prone to be misused;
signatures are best verified by trusted userspace code instead (e.g. initramfs).
But apparently there are people who use the dm-verity in-kernel signatures.  I
think systemd has some support for it, as does the recently-upstreamed IPE LSM.
I don't know what else.  The exact semantics of the "trusted" and "platform"
keyrings are not entirely clear to me, but given that dm-verity already trusts
keys from both keyrings this patch seems reasonable.  The people who actually
use this feature are in the best position to make that call, though.

- Eric

Jarkko Sakkinen Sept. 25, 2024, 7:51 a.m. UTC | #6

On Wed Sep 25, 2024 at 12:59 AM EEST, Eric Biggers wrote:
> On Wed, Sep 25, 2024 at 12:36:01AM +0300, Jarkko Sakkinen wrote:
> > On Tue Sep 24, 2024 at 9:27 PM EEST, Mikulas Patocka wrote:
> > >
> > >
> > > On Tue, 24 Sep 2024, Jarkko Sakkinen wrote:
> > >
> > > > On Mon Sep 23, 2024 at 5:04 PM EEST, Mikulas Patocka wrote:
> > > > >
> > > > >
> > > > > On Sun, 22 Sep 2024, [e-mail] wrote:
> > > > >
> > > > > > From: Luca Boccassi <[e-mail]>
> > > > > > 
> > > > > > If enabled, we fallback to the platform keyring if the trusted keyring doesn't have
> > > > > > the key used to sign the roothash. But if pkcs7_verify() rejects the key for other
> > > > > > reasons, such as usage restrictions, we do not fallback. Do so.
> > > > > > 
> > > > > > Follow-up for 6fce1f40e95182ebbfe1ee3096b8fc0b37903269
> > > > > > 
> > > > > > Suggested-by: Serge Hallyn <[e-mail]>
> > > > > > Signed-off-by: Luca Boccassi <[e-mail]>
> > > > >
> > > > > Hi
> > > > >
> > > > > I'm not an expert in keyrings.
> > > > >
> > > > > I added keyring maintainers to the CC. Please review this patch and 
> > > > > Ack/Nack it.
> > > > >
> > > > > Mikulas
> > > > >
> > > > > > ---
> > > > > >  drivers/md/dm-verity-verify-sig.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
> > > > > > index d351d7d39c60..a9e2c6c0a33c 100644
> > > > > > --- a/drivers/md/dm-verity-verify-sig.c
> > > > > > +++ b/drivers/md/dm-verity-verify-sig.c
> > > > > > @@ -127,7 +127,7 @@ int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
> > > > > >  #endif
> > > > > >  				VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
> > > > > >  #ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_PLATFORM_KEYRING
> > > > > > -	if (ret == -ENOKEY)
> > > > > > +	if (ret == -ENOKEY || ret == -EKEYREJECTED)
> > > > > >  		ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
> > > > > >  					sig_len,
> > > > > >  					VERIFY_USE_PLATFORM_KEYRING,
> > > > > > -- 
> > > > > > 2.39.5
> > > > > > 
> > > > 
> > > > I know nothing about dm-verity. What does it even do?
> > > > 
> > > > BR, Jarkko
> > >
> > > dm-verity provides a read-only device with integrity checking. dm-verity 
> > > stores hash for every block on the block device and checks the hash when 
> > > reading the block. If the hash doesn't match, it can do one of these 
> > > actions (depending on configuration):
> > > - return I/O error
> > > - try to correct the data using forward error correction
> > > - log the mismatch and do nothing
> > > - restart the machine
> > > - call panic()
> > >
> > > dm-verity is mostly used for the immutable system partition on Android 
> > > phones. For more info, see 
> > > Documentation/admin-guide/device-mapper/verity.rst
> > >
> > > The above patch changes the way that the signature of the root hash is 
> > > verified. I have no clue whether the patch can or can't subvert system 
> > > security, that's why I'd like to have some more reviews of the patch 
> > > before accepting it.
> > 
> > I guess someone who knows all this already should review it.
> > 
> > Doesn't dm-verity have a maintainer?
> > 
>
> This patch only affects dm-verity's in-kernel signature verification support,
> which has only been present since Linux v5.4 and is not used by Android or
> Chrome OS.  The whole feature seems weird to me, and it is prone to be misused;
> signatures are best verified by trusted userspace code instead (e.g. initramfs).
> But apparently there are people who use the dm-verity in-kernel signatures.  I
> think systemd has some support for it, as does the recently-upstreamed IPE LSM.
> I don't know what else.  The exact semantics of the "trusted" and "platform"
> keyrings are not entirely clear to me, but given that dm-verity already trusts
> keys from both keyrings this patch seems reasonable.  The people who actually
> use this feature are in the best position to make that call, though.

https://lwn.net/Articles/459420/

Isn't Netflix using FreeBSD these days? :-)

Right, and Chromebooks also mentioned in the same article (from 2011).

For me this looks almost like abaddonware...

>
> - Eric

BR, Jarkko

Milan Broz Sept. 25, 2024, 8:03 a.m. UTC | #7

On 9/24/24 11:59 PM, Eric Biggers wrote:
> On Wed, Sep 25, 2024 at 12:36:01AM +0300, Jarkko Sakkinen wrote:
>> On Tue Sep 24, 2024 at 9:27 PM EEST, Mikulas Patocka wrote:
>>>
>>>
>>> On Tue, 24 Sep 2024, Jarkko Sakkinen wrote:
>>>
>>>> On Mon Sep 23, 2024 at 5:04 PM EEST, Mikulas Patocka wrote:
>>>>>
>>>>>
>>>>> On Sun, 22 Sep 2024, [e-mail] wrote:
>>>>>
>>>>>> From: Luca Boccassi <[e-mail]>
>>>>>>
>>>>>> If enabled, we fallback to the platform keyring if the trusted keyring doesn't have
>>>>>> the key used to sign the roothash. But if pkcs7_verify() rejects the key for other
>>>>>> reasons, such as usage restrictions, we do not fallback. Do so.
>>>>>>
>>>>>> Follow-up for 6fce1f40e95182ebbfe1ee3096b8fc0b37903269
>>>>>>
>>>>>> Suggested-by: Serge Hallyn <[e-mail]>
>>>>>> Signed-off-by: Luca Boccassi <[e-mail]>
>>>>>
>>>>> Hi
>>>>>
>>>>> I'm not an expert in keyrings.
>>>>>
>>>>> I added keyring maintainers to the CC. Please review this patch and
>>>>> Ack/Nack it.
>>>>>
>>>>> Mikulas
>>>>>
>>>>>> ---
>>>>>>   drivers/md/dm-verity-verify-sig.c | 2 +-
>>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
>>>>>> index d351d7d39c60..a9e2c6c0a33c 100644
>>>>>> --- a/drivers/md/dm-verity-verify-sig.c
>>>>>> +++ b/drivers/md/dm-verity-verify-sig.c
>>>>>> @@ -127,7 +127,7 @@ int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
>>>>>>   #endif
>>>>>>   				VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
>>>>>>   #ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_PLATFORM_KEYRING
>>>>>> -	if (ret == -ENOKEY)
>>>>>> +	if (ret == -ENOKEY || ret == -EKEYREJECTED)
>>>>>>   		ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
>>>>>>   					sig_len,
>>>>>>   					VERIFY_USE_PLATFORM_KEYRING,
>>>>>> -- 
>>>>>> 2.39.5
>>>>>>
>>>>
>>>> I know nothing about dm-verity. What does it even do?
>>>>
>>>> BR, Jarkko
>>>
>>> dm-verity provides a read-only device with integrity checking. dm-verity
>>> stores hash for every block on the block device and checks the hash when
>>> reading the block. If the hash doesn't match, it can do one of these
>>> actions (depending on configuration):
>>> - return I/O error
>>> - try to correct the data using forward error correction
>>> - log the mismatch and do nothing
>>> - restart the machine
>>> - call panic()
>>>
>>> dm-verity is mostly used for the immutable system partition on Android
>>> phones. For more info, see
>>> Documentation/admin-guide/device-mapper/verity.rst
>>>
>>> The above patch changes the way that the signature of the root hash is
>>> verified. I have no clue whether the patch can or can't subvert system
>>> security, that's why I'd like to have some more reviews of the patch
>>> before accepting it.
>>
>> I guess someone who knows all this already should review it.
>>
>> Doesn't dm-verity have a maintainer?

(This reminds me of a nice comment from Neil about "little walled gardens" between MD & DM.
Apparently it applies to other subsystems as well. Sorry, I couldn't resist to mention it :-)

> This patch only affects dm-verity's in-kernel signature verification support,
> which has only been present since Linux v5.4 and is not used by Android or
> Chrome OS.  The whole feature seems weird to me, and it is prone to be misused;
> signatures are best verified by trusted userspace code instead (e.g. initramfs).
> But apparently there are people who use the dm-verity in-kernel signatures.  I
> think systemd has some support for it, as does the recently-upstreamed IPE LSM.
> I don't know what else.  The exact semantics of the "trusted" and "platform"
> keyrings are not entirely clear to me, but given that dm-verity already trusts
> keys from both keyrings this patch seems reasonable.  The people who actually
> use this feature are in the best position to make that call, though.

When we added support for this to veritysetup (--root-hash-signature), I think it was
a requirement from Microsoft.

Anyway, if you have a trusted key compiled-in the kernel in one keyring, I do not think
it would cause problems if stored in another.

But it scares me that we cannot easily test userspace for this in CI, as it requires compiling
own kernel with our own keys.

Do people use veritysetup (libcryptsetup) here, or does it run with its separate userspace tooling?

Milan

Jarkko Sakkinen Sept. 25, 2024, 9:05 a.m. UTC | #8

On Wed Sep 25, 2024 at 11:03 AM EEST, Milan Broz wrote:
> >> Doesn't dm-verity have a maintainer?
>
> (This reminds me of a nice comment from Neil about "little walled
> gardens" between MD & DM.  Apparently it applies to other subsystems
> as well. Sorry, I couldn't resist to mention it :-)

Np, it's just that last and only time I've ever read anything about
dm-verity was 2011 article :-)

I will rephrase question: does dm-verity have a user? ;-)

BR, Jarkko

Serge E. Hallyn Sept. 25, 2024, 12:57 p.m. UTC | #9

On Wed, Sep 25, 2024 at 12:05:59PM +0300, Jarkko Sakkinen wrote:
> On Wed Sep 25, 2024 at 11:03 AM EEST, Milan Broz wrote:
> > >> Doesn't dm-verity have a maintainer?
> >
> > (This reminds me of a nice comment from Neil about "little walled
> > gardens" between MD & DM.  Apparently it applies to other subsystems
> > as well. Sorry, I couldn't resist to mention it :-)
> 
> Np, it's just that last and only time I've ever read anything about
> dm-verity was 2011 article :-)
> 
> I will rephrase question: does dm-verity have a user? ;-)

It gets used for integrity guarantees in certain containers, where
the layers of tarballs are replaced by layers of squashfs, with the
dmverity root hash for each layer listed in the signed manifest, e.g.

github.com/project-stacker/stacker
github.com/project-machine/atomfs

This is used of course to verify container integrity, and also gets used by
some projects and products to create an RFS from such images during initrd

github.com/project-machine/mos

-serge

Jarkko Sakkinen Sept. 25, 2024, 2:50 p.m. UTC | #10

On Wed Sep 25, 2024 at 3:57 PM EEST, Serge E. Hallyn wrote:
> On Wed, Sep 25, 2024 at 12:05:59PM +0300, Jarkko Sakkinen wrote:
> > On Wed Sep 25, 2024 at 11:03 AM EEST, Milan Broz wrote:
> > > >> Doesn't dm-verity have a maintainer?
> > >
> > > (This reminds me of a nice comment from Neil about "little walled
> > > gardens" between MD & DM.  Apparently it applies to other subsystems
> > > as well. Sorry, I couldn't resist to mention it :-)
> > 
> > Np, it's just that last and only time I've ever read anything about
> > dm-verity was 2011 article :-)
> > 
> > I will rephrase question: does dm-verity have a user? ;-)
>
> It gets used for integrity guarantees in certain containers, where
> the layers of tarballs are replaced by layers of squashfs, with the
> dmverity root hash for each layer listed in the signed manifest, e.g.
>
> github.com/project-stacker/stacker
> github.com/project-machine/atomfs
>
> This is used of course to verify container integrity, and also gets used by
> some projects and products to create an RFS from such images during initrd
>
> github.com/project-machine/mos

OK got it!

I did some studying and query and to put short it is a merkle tree
for rootfs for devices like phones and tablets for instance. I.e.
when you modify only on "system update". 

So... let's check the mainatainers list:

❯ scripts/get_maintainer.pl drivers/md/dm-verity-verify-sig.c
Alasdair Kergon <[e-mail]> (maintainer:DEVICE-MAPPER  (LVM))
Mike Snitzer <[e-mail]> (maintainer:DEVICE-MAPPER  (LVM))
Mikulas Patocka <[e-mail]> (maintainer:DEVICE-MAPPER  (LVM))
[e-mail] (open list:DEVICE-MAPPER  (LVM))
[e-mail] (open list)

Mikulas, I guess you take care of this if I just ack the return value?

If that holds, and given that I actually know what verify_pkcs7_signature()
does, I think the code patch makes sense to me, and thus:

Acked-by: Jarkko Sakkinen <[e-mail]>

I.e. I think it uses API correctly.

>
> -serge

BR, Jarkko

Mikulas Patocka Sept. 25, 2024, 3:37 p.m. UTC | #11

On Sun, 22 Sep 2024, [e-mail] wrote:

> From: Luca Boccassi <[e-mail]>
> 
> If enabled, we fallback to the platform keyring if the trusted keyring doesn't have
> the key used to sign the roothash. But if pkcs7_verify() rejects the key for other
> reasons, such as usage restrictions, we do not fallback. Do so.
> 
> Follow-up for 6fce1f40e95182ebbfe1ee3096b8fc0b37903269
> 
> Suggested-by: Serge Hallyn <[e-mail]>
> Signed-off-by: Luca Boccassi <[e-mail]>
> ---
>  drivers/md/dm-verity-verify-sig.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
> index d351d7d39c60..a9e2c6c0a33c 100644
> --- a/drivers/md/dm-verity-verify-sig.c
> +++ b/drivers/md/dm-verity-verify-sig.c
> @@ -127,7 +127,7 @@ int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
>  #endif
>  				VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
>  #ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_PLATFORM_KEYRING
> -	if (ret == -ENOKEY)
> +	if (ret == -ENOKEY || ret == -EKEYREJECTED)
>  		ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
>  					sig_len,
>  					VERIFY_USE_PLATFORM_KEYRING,
> -- 
> 2.39.5

Hi

Please describe what problem does this patch solve. I.e. why would anyone 
put a key into the trusted keyring that could sign the roothash and 
restrict its usage so that it can't be used to sign the roothash?

In the other places of the kernel, only -ENOKEY is tested:
kexec_kernel_verify_pe_sig:
if (ret == -ENOKEY && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING))
s390_verify_sig:
if (ret == -ENOKEY && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING))
do they need to be converted to -EKEYREJECTED too?

Mikulas

Eric Biggers Sept. 25, 2024, 4:53 p.m. UTC | #12

On Wed, Sep 25, 2024 at 12:05:59PM +0300, Jarkko Sakkinen wrote:
> On Wed Sep 25, 2024 at 11:03 AM EEST, Milan Broz wrote:
> > >> Doesn't dm-verity have a maintainer?
> >
> > (This reminds me of a nice comment from Neil about "little walled
> > gardens" between MD & DM.  Apparently it applies to other subsystems
> > as well. Sorry, I couldn't resist to mention it :-)
> 
> Np, it's just that last and only time I've ever read anything about
> dm-verity was 2011 article :-)
> 
> I will rephrase question: does dm-verity have a user? ;-)
> 
> BR, Jarkko

Sorry if I was unclear.  dm-verity is widely used, including by all Android and
Chrome OS devices.  But this patch is about dm-verity's in-kernel signature
verification which is an optional sub-feature that is not widely used.  That
sub-feature is apparently difficult to test and not clearly specified, which is
why people seem to be struggling a bit with this patch.

- Eric

Jarkko Sakkinen Sept. 25, 2024, 5:15 p.m. UTC | #13

On Wed Sep 25, 2024 at 7:53 PM EEST, Eric Biggers wrote:
> On Wed, Sep 25, 2024 at 12:05:59PM +0300, Jarkko Sakkinen wrote:
> > On Wed Sep 25, 2024 at 11:03 AM EEST, Milan Broz wrote:
> > > >> Doesn't dm-verity have a maintainer?
> > >
> > > (This reminds me of a nice comment from Neil about "little walled
> > > gardens" between MD & DM.  Apparently it applies to other subsystems
> > > as well. Sorry, I couldn't resist to mention it :-)
> > 
> > Np, it's just that last and only time I've ever read anything about
> > dm-verity was 2011 article :-)
> > 
> > I will rephrase question: does dm-verity have a user? ;-)
> > 
> > BR, Jarkko
>
> Sorry if I was unclear.  dm-verity is widely used, including by all Android and
> Chrome OS devices.  But this patch is about dm-verity's in-kernel signature
> verification which is an optional sub-feature that is not widely used.  That
> sub-feature is apparently difficult to test and not clearly specified, which is
> why people seem to be struggling a bit with this patch.

NP, I learned a new thing ;-)

Before Linux I worked with Symbian (ugh) so this whole scheme for doing
FW updates is familiar to me from the dark ages...

And I acked the change too!

> - Eric

BR, Jarkko

Luca Boccassi Sept. 25, 2024, 9:28 p.m. UTC | #14

On Wed, 25 Sept 2024 at 10:03, Milan Broz <[e-mail]> wrote:
>
> On 9/24/24 11:59 PM, Eric Biggers wrote:
> > On Wed, Sep 25, 2024 at 12:36:01AM +0300, Jarkko Sakkinen wrote:
> >> On Tue Sep 24, 2024 at 9:27 PM EEST, Mikulas Patocka wrote:
> >>>
> >>>
> >>> On Tue, 24 Sep 2024, Jarkko Sakkinen wrote:
> >>>
> >>>> On Mon Sep 23, 2024 at 5:04 PM EEST, Mikulas Patocka wrote:
> >>>>>
> >>>>>
> >>>>> On Sun, 22 Sep 2024, [e-mail] wrote:
> >>>>>
> >>>>>> From: Luca Boccassi <[e-mail]>
> >>>>>>
> >>>>>> If enabled, we fallback to the platform keyring if the trusted keyring doesn't have
> >>>>>> the key used to sign the roothash. But if pkcs7_verify() rejects the key for other
> >>>>>> reasons, such as usage restrictions, we do not fallback. Do so.
> >>>>>>
> >>>>>> Follow-up for 6fce1f40e95182ebbfe1ee3096b8fc0b37903269
> >>>>>>
> >>>>>> Suggested-by: Serge Hallyn <[e-mail]>
> >>>>>> Signed-off-by: Luca Boccassi <[e-mail]>
> >>>>>
> >>>>> Hi
> >>>>>
> >>>>> I'm not an expert in keyrings.
> >>>>>
> >>>>> I added keyring maintainers to the CC. Please review this patch and
> >>>>> Ack/Nack it.
> >>>>>
> >>>>> Mikulas
> >>>>>
> >>>>>> ---
> >>>>>>   drivers/md/dm-verity-verify-sig.c | 2 +-
> >>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
> >>>>>> index d351d7d39c60..a9e2c6c0a33c 100644
> >>>>>> --- a/drivers/md/dm-verity-verify-sig.c
> >>>>>> +++ b/drivers/md/dm-verity-verify-sig.c
> >>>>>> @@ -127,7 +127,7 @@ int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
> >>>>>>   #endif
> >>>>>>                                  VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
> >>>>>>   #ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_PLATFORM_KEYRING
> >>>>>> -        if (ret == -ENOKEY)
> >>>>>> +        if (ret == -ENOKEY || ret == -EKEYREJECTED)
> >>>>>>                  ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
> >>>>>>                                          sig_len,
> >>>>>>                                          VERIFY_USE_PLATFORM_KEYRING,
> >>>>>> --
> >>>>>> 2.39.5
> >>>>>>
> >>>>
> >>>> I know nothing about dm-verity. What does it even do?
> >>>>
> >>>> BR, Jarkko
> >>>
> >>> dm-verity provides a read-only device with integrity checking. dm-verity
> >>> stores hash for every block on the block device and checks the hash when
> >>> reading the block. If the hash doesn't match, it can do one of these
> >>> actions (depending on configuration):
> >>> - return I/O error
> >>> - try to correct the data using forward error correction
> >>> - log the mismatch and do nothing
> >>> - restart the machine
> >>> - call panic()
> >>>
> >>> dm-verity is mostly used for the immutable system partition on Android
> >>> phones. For more info, see
> >>> Documentation/admin-guide/device-mapper/verity.rst
> >>>
> >>> The above patch changes the way that the signature of the root hash is
> >>> verified. I have no clue whether the patch can or can't subvert system
> >>> security, that's why I'd like to have some more reviews of the patch
> >>> before accepting it.
> >>
> >> I guess someone who knows all this already should review it.
> >>
> >> Doesn't dm-verity have a maintainer?
>
> (This reminds me of a nice comment from Neil about "little walled gardens" between MD & DM.
> Apparently it applies to other subsystems as well. Sorry, I couldn't resist to mention it :-)
>
> > This patch only affects dm-verity's in-kernel signature verification support,
> > which has only been present since Linux v5.4 and is not used by Android or
> > Chrome OS.  The whole feature seems weird to me, and it is prone to be misused;
> > signatures are best verified by trusted userspace code instead (e.g. initramfs).
> > But apparently there are people who use the dm-verity in-kernel signatures.  I
> > think systemd has some support for it, as does the recently-upstreamed IPE LSM.
> > I don't know what else.  The exact semantics of the "trusted" and "platform"
> > keyrings are not entirely clear to me, but given that dm-verity already trusts
> > keys from both keyrings this patch seems reasonable.  The people who actually
> > use this feature are in the best position to make that call, though.
>
> When we added support for this to veritysetup (--root-hash-signature), I think it was
> a requirement from Microsoft.
>
> Anyway, if you have a trusted key compiled-in the kernel in one keyring, I do not think
> it would cause problems if stored in another.
>
> But it scares me that we cannot easily test userspace for this in CI, as it requires compiling
> own kernel with our own keys.
>
> Do people use veritysetup (libcryptsetup) here, or does it run with its separate userspace tooling?

This is used with libcryptsetup commonly, and often with veritysetup.
It is fairly easy to test in a VM or on baremetal, it is not required
to build your own kernel - that's the reason for supporting
secondary+platform keyrings (the first one allows you to enroll keys
in MOK, the second one for UEFI DB).
We would even have a CI testing this for every PR and merge in systemd
on Github, _except_ there is currently an issue (unrelated to
dmverity) that happens when nesting KVM with UEFI secure boot enabled
on top of HyperV, which means it cannot be used reliably on Github
Actions. Once that is solved, this will be again part of the systemd
CI integration tests. But it is used regularly by developers on their
machines.

It might not be commonly used by kernel developers, I do not know as I
am not a kernel developer, but it is becoming more and more common in
userspace and among image builders. For example the mkosi image
builder, using systemd-repart, can very easily build distro images
using signed dm verity. I am at All Systems Go and just today there
were multiple talks by multiple people using dmverity images for their
distros/platforms/products, especially with systemd-sysext, which is
all about signed dm-verity.

In 6.12 we will also have IPE which allows to enable trusted code
integrity checks that cannot be trivially bypassed by other userspace
processes running with root or caps. This has been, still is and will
be for the foreseeable future, in use in the Azure infrastructure.

Hope this provides some clarity, let me know if you need more info.

Luca Boccassi Sept. 25, 2024, 9:37 p.m. UTC | #15

On Wed, 25 Sept 2024 at 17:38, Mikulas Patocka <[e-mail]> wrote:
>
>
>
> On Sun, 22 Sep 2024, [e-mail] wrote:
>
> > From: Luca Boccassi <[e-mail]>
> >
> > If enabled, we fallback to the platform keyring if the trusted keyring doesn't have
> > the key used to sign the roothash. But if pkcs7_verify() rejects the key for other
> > reasons, such as usage restrictions, we do not fallback. Do so.
> >
> > Follow-up for 6fce1f40e95182ebbfe1ee3096b8fc0b37903269
> >
> > Suggested-by: Serge Hallyn <[e-mail]>
> > Signed-off-by: Luca Boccassi <[e-mail]>
> > ---
> >  drivers/md/dm-verity-verify-sig.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
> > index d351d7d39c60..a9e2c6c0a33c 100644
> > --- a/drivers/md/dm-verity-verify-sig.c
> > +++ b/drivers/md/dm-verity-verify-sig.c
> > @@ -127,7 +127,7 @@ int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
> >  #endif
> >                               VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
> >  #ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_PLATFORM_KEYRING
> > -     if (ret == -ENOKEY)
> > +     if (ret == -ENOKEY || ret == -EKEYREJECTED)
> >               ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
> >                                       sig_len,
> >                                       VERIFY_USE_PLATFORM_KEYRING,
> > --
> > 2.39.5
>
> Hi
>
> Please describe what problem does this patch solve. I.e. why would anyone
> put a key into the trusted keyring that could sign the roothash and
> restrict its usage so that it can't be used to sign the roothash?
>
> In the other places of the kernel, only -ENOKEY is tested:
> kexec_kernel_verify_pe_sig:
> if (ret == -ENOKEY && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING))
> s390_verify_sig:
> if (ret == -ENOKEY && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING))
> do they need to be converted to -EKEYREJECTED too?

To be perfectly honest, I do not have a use case for the EKEYREJECTED
fallback - it was suggested by Serge when I was working on the IPE
keyrings usage:

https://lore.kernel.org/all/[e-mail]/

I would like to maintain the usage of keyrings in sync between the two
components given they are used together, for consistency, so I
counter-proposed to send the change to dmverity first. I am following
up on that promise with this patch.
If there are reasons not to take this change, or if there are no
convincing reasons to take it, I do not have strong opinions either
way so I don't mind what the outcome is either way, as I am just
following up on that review comment. I'll let Serge explain the
reasoning for the request, if he wants to do it.

If this change is accepted, I will send the same for IPE. If not, I
won't. I do not directly use other subsystems that use these keyrings,
so I will not send changes for those even if this goes in.

Mikulas Patocka Sept. 26, 2024, 3:02 p.m. UTC | #16

On Wed, 25 Sep 2024, Luca Boccassi wrote:

> > > diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
> > > index d351d7d39c60..a9e2c6c0a33c 100644
> > > --- a/drivers/md/dm-verity-verify-sig.c
> > > +++ b/drivers/md/dm-verity-verify-sig.c
> > > @@ -127,7 +127,7 @@ int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
> > >  #endif
> > >                               VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
> > >  #ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_PLATFORM_KEYRING
> > > -     if (ret == -ENOKEY)
> > > +     if (ret == -ENOKEY || ret == -EKEYREJECTED)
> > >               ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
> > >                                       sig_len,
> > >                                       VERIFY_USE_PLATFORM_KEYRING,
> > > --
> > > 2.39.5
> >
> > Hi
> >
> > Please describe what problem does this patch solve. I.e. why would anyone
> > put a key into the trusted keyring that could sign the roothash and
> > restrict its usage so that it can't be used to sign the roothash?
> >
> > In the other places of the kernel, only -ENOKEY is tested:
> > kexec_kernel_verify_pe_sig:
> > if (ret == -ENOKEY && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING))
> > s390_verify_sig:
> > if (ret == -ENOKEY && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING))
> > do they need to be converted to -EKEYREJECTED too?
> 
> To be perfectly honest, I do not have a use case for the EKEYREJECTED
> fallback - it was suggested by Serge when I was working on the IPE
> keyrings usage:
> 
> https://lore.kernel.org/all/[e-mail]/
> 
> I would like to maintain the usage of keyrings in sync between the two
> components given they are used together, for consistency, so I
> counter-proposed to send the change to dmverity first. I am following
> up on that promise with this patch.
> If there are reasons not to take this change, or if there are no
> convincing reasons to take it, I do not have strong opinions either
> way so I don't mind what the outcome is either way, as I am just
> following up on that review comment. I'll let Serge explain the
> reasoning for the request, if he wants to do it.
> 
> If this change is accepted, I will send the same for IPE. If not, I
> won't. I do not directly use other subsystems that use these keyrings,
> so I will not send changes for those even if this goes in.

Hi

I accepted this patch, I'll send it to Linus with other device mapper 
patches.

Mikulas

Luca Boccassi Sept. 26, 2024, 3:41 p.m. UTC | #17

On Thu, 26 Sept 2024 at 17:02, Mikulas Patocka <[e-mail]> wrote:
>
>
>
> On Wed, 25 Sep 2024, Luca Boccassi wrote:
>
> > > > diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
> > > > index d351d7d39c60..a9e2c6c0a33c 100644
> > > > --- a/drivers/md/dm-verity-verify-sig.c
> > > > +++ b/drivers/md/dm-verity-verify-sig.c
> > > > @@ -127,7 +127,7 @@ int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
> > > >  #endif
> > > >                               VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
> > > >  #ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_PLATFORM_KEYRING
> > > > -     if (ret == -ENOKEY)
> > > > +     if (ret == -ENOKEY || ret == -EKEYREJECTED)
> > > >               ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
> > > >                                       sig_len,
> > > >                                       VERIFY_USE_PLATFORM_KEYRING,
> > > > --
> > > > 2.39.5
> > >
> > > Hi
> > >
> > > Please describe what problem does this patch solve. I.e. why would anyone
> > > put a key into the trusted keyring that could sign the roothash and
> > > restrict its usage so that it can't be used to sign the roothash?
> > >
> > > In the other places of the kernel, only -ENOKEY is tested:
> > > kexec_kernel_verify_pe_sig:
> > > if (ret == -ENOKEY && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING))
> > > s390_verify_sig:
> > > if (ret == -ENOKEY && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING))
> > > do they need to be converted to -EKEYREJECTED too?
> >
> > To be perfectly honest, I do not have a use case for the EKEYREJECTED
> > fallback - it was suggested by Serge when I was working on the IPE
> > keyrings usage:
> >
> > https://lore.kernel.org/all/[e-mail]/
> >
> > I would like to maintain the usage of keyrings in sync between the two
> > components given they are used together, for consistency, so I
> > counter-proposed to send the change to dmverity first. I am following
> > up on that promise with this patch.
> > If there are reasons not to take this change, or if there are no
> > convincing reasons to take it, I do not have strong opinions either
> > way so I don't mind what the outcome is either way, as I am just
> > following up on that review comment. I'll let Serge explain the
> > reasoning for the request, if he wants to do it.
> >
> > If this change is accepted, I will send the same for IPE. If not, I
> > won't. I do not directly use other subsystems that use these keyrings,
> > so I will not send changes for those even if this goes in.
>
> Hi
>
> I accepted this patch, I'll send it to Linus with other device mapper
> patches.
>
> Mikulas

Thank you!

Milan Broz Sept. 27, 2024, 7:12 a.m. UTC | #18

On 9/25/24 11:28 PM, Luca Boccassi wrote:
>>
>> Do people use veritysetup (libcryptsetup) here, or does it run with its separate userspace tooling?
> 
> This is used with libcryptsetup commonly, and often with veritysetup.
> It is fairly easy to test in a VM or on baremetal, it is not required
> to build your own kernel - that's the reason for supporting
> secondary+platform keyrings (the first one allows you to enroll keys
> in MOK, the second one for UEFI DB).
> We would even have a CI testing this for every PR and merge in systemd
> on Github, _except_ there is currently an issue (unrelated to
> dmverity) that happens when nesting KVM with UEFI secure boot enabled
> on top of HyperV, which means it cannot be used reliably on Github
> Actions. Once that is solved, this will be again part of the systemd
> CI integration tests. But it is used regularly by developers on their
> machines.

Hi Luca,

good to know that libcryptswtup userspace is then used here, thanks for the info!

I have some more questions, but that is not related to this thread,
I will ask in another mail later.

Thanks,
Milan


> 
> It might not be commonly used by kernel developers, I do not know as I
> am not a kernel developer, but it is becoming more and more common in
> userspace and among image builders. For example the mkosi image
> builder, using systemd-repart, can very easily build distro images
> using signed dm verity. I am at All Systems Go and just today there
> were multiple talks by multiple people using dmverity images for their
> distros/platforms/products, especially with systemd-sysext, which is
> all about signed dm-verity.
> 
> In 6.12 we will also have IPE which allows to enable trusted code
> integrity checks that cannot be trivially bypassed by other userspace
> processes running with root or caps. This has been, still is and will
> be for the foreseeable future, in use in the Azure infrastructure.
> 
> Hope this provides some clarity, let me know if you need more info.

Patch

diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
index d351d7d39c60..a9e2c6c0a33c 100644
--- a/drivers/md/dm-verity-verify-sig.c
+++ b/drivers/md/dm-verity-verify-sig.c
@@ -127,7 +127,7 @@  int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
 #endif
 				VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
 #ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_PLATFORM_KEYRING
-	if (ret == -ENOKEY)
+	if (ret == -ENOKEY || ret == -EKEYREJECTED)
 		ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
 					sig_len,
 					VERIFY_USE_PLATFORM_KEYRING,