Re: [PATCH] dm verity: fallback to platform keyring also if key in trusted keyring is rejected

Re: [PATCH] dm verity: fallback to platform keyring also if key in trusted keyring is rejected

[Mikulas Patocka]

On Sun, 22 Sep 2024, luca.boccassi@gmail.com wrote:

> From: Luca Boccassi <bluca@debian.org>
> 
> 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 <serge@hallyn.com>
> Signed-off-by: Luca Boccassi <bluca@debian.org>

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
> 

Re: [PATCH] dm verity: fallback to platform keyring also if key in trusted keyring is rejected

[Jarkko Sakkinen]

On Mon Sep 23, 2024 at 5:04 PM EEST, Mikulas Patocka wrote:
>
>
> On Sun, 22 Sep 2024, luca.boccassi@gmail.com wrote:
>
> > From: Luca Boccassi <bluca@debian.org>
> > 
> > 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 <serge@hallyn.com>
> > Signed-off-by: Luca Boccassi <bluca@debian.org>
>
> 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

Re: [PATCH] dm verity: fallback to platform keyring also if key in trusted keyring is rejected

[Mikulas Patocka]

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, luca.boccassi@gmail.com wrote:
> >
> > > From: Luca Boccassi <bluca@debian.org>
> > > 
> > > 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 <serge@hallyn.com>
> > > Signed-off-by: Luca Boccassi <bluca@debian.org>
> >
> > 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

Re: [PATCH] dm verity: fallback to platform keyring also if key in trusted keyring is rejected

[Jarkko Sakkinen]

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, luca.boccassi@gmail.com wrote:
> > >
> > > > From: Luca Boccassi <bluca@debian.org>
> > > > 
> > > > 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 <serge@hallyn.com>
> > > > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > >
> > > 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

Re: [PATCH] dm verity: fallback to platform keyring also if key in trusted keyring is rejected

[Eric Biggers]

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, luca.boccassi@gmail.com wrote:
> > > >
> > > > > From: Luca Boccassi <bluca@debian.org>
> > > > > 
> > > > > 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 <serge@hallyn.com>
> > > > > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > > >
> > > > 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

Re: [PATCH] dm verity: fallback to platform keyring also if key in trusted keyring is rejected

[Jarkko Sakkinen]

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, luca.boccassi@gmail.com wrote:
> > > > >
> > > > > > From: Luca Boccassi <bluca@debian.org>
> > > > > > 
> > > > > > 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 <serge@hallyn.com>
> > > > > > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > > > >
> > > > > 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

Re: [PATCH] dm verity: fallback to platform keyring also if key in trusted keyring is rejected

[Milan Broz]

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, luca.boccassi@gmail.com wrote:
>>>>>
>>>>>> From: Luca Boccassi <bluca@debian.org>
>>>>>>
>>>>>> 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 <serge@hallyn.com>
>>>>>> Signed-off-by: Luca Boccassi <bluca@debian.org>
>>>>>
>>>>> 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

Re: [PATCH] dm verity: fallback to platform keyring also if key in trusted keyring is rejected

[Jarkko Sakkinen]

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

Re: [PATCH] dm verity: fallback to platform keyring also if key in trusted keyring is rejected

[Serge E. Hallyn]

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

Re: [PATCH] dm verity: fallback to platform keyring also if key in trusted keyring is rejected

[Jarkko Sakkinen]

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 <agk@redhat.com> (maintainer:DEVICE-MAPPER  (LVM))
Mike Snitzer <snitzer@kernel.org> (maintainer:DEVICE-MAPPER  (LVM))
Mikulas Patocka <mpatocka@redhat.com> (maintainer:DEVICE-MAPPER  (LVM))
dm-devel@lists.linux.dev (open list:DEVICE-MAPPER  (LVM))
linux-kernel@vger.kernel.org (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 <jarkko@kernel.org>

I.e. I think it uses API correctly.

>
> -serge

BR, Jarkko

Re: [PATCH] dm verity: fallback to platform keyring also if key in trusted keyring is rejected

[Eric Biggers]

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

Re: [PATCH] dm verity: fallback to platform keyring also if key in trusted keyring is rejected

[Jarkko Sakkinen]

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

Re: [PATCH] dm verity: fallback to platform keyring also if key in trusted keyring is rejected

[Luca Boccassi]

On Wed, 25 Sept 2024 at 10:03, Milan Broz <gmazyland@gmail.com> 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, luca.boccassi@gmail.com wrote:
> >>>>>
> >>>>>> From: Luca Boccassi <bluca@debian.org>
> >>>>>>
> >>>>>> 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 <serge@hallyn.com>
> >>>>>> Signed-off-by: Luca Boccassi <bluca@debian.org>
> >>>>>
> >>>>> 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.

Re: [PATCH] dm verity: fallback to platform keyring also if key in trusted keyring is rejected

[Milan Broz]

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.