[v2] dm verity: Add support for signature verification with 2nd keyring

Commit Message

Mickaël Salaün Oct. 15, 2020, 3:05 p.m. UTC

From: Mickaël Salaün <[e-mail]>

Add a new configuration DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING
to enable dm-verity signatures to be verified against the secondary
trusted keyring.  Instead of relying on the builtin trusted keyring
(with hard-coded certificates), the second trusted keyring can include
certificate authorities from the builtin trusted keyring and child
certificates loaded at run time.  Using the secondary trusted keyring
enables to use dm-verity disks (e.g. loop devices) signed by keys which
did not exist at kernel build time, leveraging the certificate chain of
trust model.  In practice, this makes it possible to update certificates
without kernel update and reboot, aligning with module and kernel
(kexec) signature verification which already use the secondary trusted
keyring.

Cc: Alasdair Kergon <[e-mail]>
Cc: Andrew Morton <[e-mail]>
Cc: Jarkko Sakkinen <[e-mail]>
Cc: Jaskaran Khurana <[e-mail]>
Cc: Mike Snitzer <[e-mail]>
Cc: Milan Broz <[e-mail]>
Signed-off-by: Mickaël Salaün <[e-mail]>
---

Previous version:
https://lore.kernel.org/lkml/[e-mail]/

Changes since v1:
* Extend the commit message (asked by Jarkko Sakkinen).
* Rename the Kconfig "help" keyword according to commit 84af7a6194e4
  ("checkpatch: kconfig: prefer 'help' over '---help---'").
---
 drivers/md/Kconfig                | 13 ++++++++++++-
 drivers/md/dm-verity-verify-sig.c |  9 +++++++--
 2 files changed, 19 insertions(+), 3 deletions(-)


base-commit: bbf5c979011a099af5dc76498918ed7df445635b

Comments

Mike Snitzer Oct. 15, 2020, 4:52 p.m. UTC | #1

On Thu, Oct 15 2020 at 11:05am -0400,
Mickaël Salaün <[e-mail]> wrote:

> From: Mickaël Salaün <[e-mail]>
> 
> Add a new configuration DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING
> to enable dm-verity signatures to be verified against the secondary
> trusted keyring.  Instead of relying on the builtin trusted keyring
> (with hard-coded certificates), the second trusted keyring can include
> certificate authorities from the builtin trusted keyring and child
> certificates loaded at run time.  Using the secondary trusted keyring
> enables to use dm-verity disks (e.g. loop devices) signed by keys which
> did not exist at kernel build time, leveraging the certificate chain of
> trust model.  In practice, this makes it possible to update certificates
> without kernel update and reboot, aligning with module and kernel
> (kexec) signature verification which already use the secondary trusted
> keyring.
> 
> Cc: Alasdair Kergon <[e-mail]>
> Cc: Andrew Morton <[e-mail]>
> Cc: Jarkko Sakkinen <[e-mail]>
> Cc: Jaskaran Khurana <[e-mail]>
> Cc: Mike Snitzer <[e-mail]>
> Cc: Milan Broz <[e-mail]>
> Signed-off-by: Mickaël Salaün <[e-mail]>
> ---
> 
> Previous version:
> https://lore.kernel.org/lkml/[e-mail]/
> 
> Changes since v1:
> * Extend the commit message (asked by Jarkko Sakkinen).
> * Rename the Kconfig "help" keyword according to commit 84af7a6194e4
>   ("checkpatch: kconfig: prefer 'help' over '---help---'").

Can you please explain why you've decided to make this a Kconfig CONFIG
knob?  Why not either add: a dm-verity table argument? A dm-verity
kernel module parameter? or both (to allow a particular default but then
per-device override)?

Otherwise, _all_ DM verity devices will be configured to use secondary
keyring fallback.  Is that really desirable?

Regardless, I really don't see why a Kconfig knob is appropriate.

Mike


> ---
>  drivers/md/Kconfig                | 13 ++++++++++++-
>  drivers/md/dm-verity-verify-sig.c |  9 +++++++--
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
> index 30ba3573626c..1d68935e45ef 100644
> --- a/drivers/md/Kconfig
> +++ b/drivers/md/Kconfig
> @@ -530,11 +530,22 @@ config DM_VERITY_VERIFY_ROOTHASH_SIG
>  	bool "Verity data device root hash signature verification support"
>  	depends on DM_VERITY
>  	select SYSTEM_DATA_VERIFICATION
> -	  help
> +	help
>  	  Add ability for dm-verity device to be validated if the
>  	  pre-generated tree of cryptographic checksums passed has a pkcs#7
>  	  signature file that can validate the roothash of the tree.
>  
> +	  By default, rely on the builtin trusted keyring.
> +
> +	  If unsure, say N.
> +
> +config DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING
> +	bool "Verity data device root hash signature verification with secondary keyring"
> +	depends on DM_VERITY_VERIFY_ROOTHASH_SIG
> +	depends on SECONDARY_TRUSTED_KEYRING
> +	help
> +	  Rely on the secondary trusted keyring to verify dm-verity signatures.
> +
>  	  If unsure, say N.
>  
>  config DM_VERITY_FEC
> diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
> index 614e43db93aa..29385dc470d5 100644
> --- a/drivers/md/dm-verity-verify-sig.c
> +++ b/drivers/md/dm-verity-verify-sig.c
> @@ -119,8 +119,13 @@ int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
>  	}
>  
>  	ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
> -				sig_len, NULL, VERIFYING_UNSPECIFIED_SIGNATURE,
> -				NULL, NULL);
> +				sig_len,
> +#ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING
> +				VERIFY_USE_SECONDARY_KEYRING,
> +#else
> +				NULL,
> +#endif
> +				VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
>  
>  	return ret;
>  }
> 
> base-commit: bbf5c979011a099af5dc76498918ed7df445635b
> -- 
> 2.28.0
>

Mickaël Salaün Oct. 16, 2020, 8:29 a.m. UTC | #2

On 15/10/2020 18:52, Mike Snitzer wrote:
> On Thu, Oct 15 2020 at 11:05am -0400,
> Mickaël Salaün <[e-mail]> wrote:
> 
>> From: Mickaël Salaün <[e-mail]>
>>
>> Add a new configuration DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING
>> to enable dm-verity signatures to be verified against the secondary
>> trusted keyring.  Instead of relying on the builtin trusted keyring
>> (with hard-coded certificates), the second trusted keyring can include
>> certificate authorities from the builtin trusted keyring and child
>> certificates loaded at run time.  Using the secondary trusted keyring
>> enables to use dm-verity disks (e.g. loop devices) signed by keys which
>> did not exist at kernel build time, leveraging the certificate chain of
>> trust model.  In practice, this makes it possible to update certificates
>> without kernel update and reboot, aligning with module and kernel
>> (kexec) signature verification which already use the secondary trusted
>> keyring.
>>
>> Cc: Alasdair Kergon <[e-mail]>
>> Cc: Andrew Morton <[e-mail]>
>> Cc: Jarkko Sakkinen <[e-mail]>
>> Cc: Jaskaran Khurana <[e-mail]>
>> Cc: Mike Snitzer <[e-mail]>
>> Cc: Milan Broz <[e-mail]>
>> Signed-off-by: Mickaël Salaün <[e-mail]>
>> ---
>>
>> Previous version:
>> https://lore.kernel.org/lkml/[e-mail]/
>>
>> Changes since v1:
>> * Extend the commit message (asked by Jarkko Sakkinen).
>> * Rename the Kconfig "help" keyword according to commit 84af7a6194e4
>>   ("checkpatch: kconfig: prefer 'help' over '---help---'").
> 
> Can you please explain why you've decided to make this a Kconfig CONFIG
> knob?  Why not either add: a dm-verity table argument? A dm-verity
> kernel module parameter? or both (to allow a particular default but then
> per-device override)?

The purpose of signed dm-verity images is to authenticate files, or said
in another way, to enable the kernel to trust disk images in a flexible
way (i.e. thanks to certificate's chain of trust). Being able to update
such chain at run time requires to use the second trusted keyring. This
keyring automatically includes the certificate authorities from the
builtin trusted keyring, which are required to dynamically populate the
secondary trusted keyring with certificates signed by an already trusted
authority. The roots of trust must then be included at build time in the
builtin trusted keyring.

To be meaningful, using dm-verity signatures implies to have a
restricted user space, i.e. even the root user has limited power over
the kernel and the rest of the system. Blindly trusting data provided by
user space (e.g. dm-verity table argument, kernel module parameter)
defeat the purpose of (mandatory) authenticated images.

> 
> Otherwise, _all_ DM verity devices will be configured to use secondary
> keyring fallback.  Is that really desirable?

That is already the current state (on purpose).

> 
> Regardless, I really don't see why a Kconfig knob is appropriate.

Moreover, a Kconfig knob makes sense as much as
DM_VERITY_VERIFY_ROOTHASH_SIG,
IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY, MODULE_SIG_FORCE and
other similar authentication mechanisms. Indeed, when using these
configurations, we want the kernel to enforce a specific policy.

Obviously, we can't make the DM_VERITY_VERIFY_ROOTHASH_SIG relies on the
secondary trusted keyring without important security implications for
systems already using this configuration (and then the builtin trusted
keyring as the unique source of trust).

Mickaël Salaün Oct. 16, 2020, 8:49 a.m. UTC | #3

On 16/10/2020 10:29, Mickaël Salaün wrote:
> 
> On 15/10/2020 18:52, Mike Snitzer wrote:
>> On Thu, Oct 15 2020 at 11:05am -0400,
>> Mickaël Salaün <[e-mail]> wrote:
>>
>>> From: Mickaël Salaün <[e-mail]>
>>>
>>> Add a new configuration DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING
>>> to enable dm-verity signatures to be verified against the secondary
>>> trusted keyring.  Instead of relying on the builtin trusted keyring
>>> (with hard-coded certificates), the second trusted keyring can include
>>> certificate authorities from the builtin trusted keyring and child
>>> certificates loaded at run time.  Using the secondary trusted keyring
>>> enables to use dm-verity disks (e.g. loop devices) signed by keys which
>>> did not exist at kernel build time, leveraging the certificate chain of
>>> trust model.  In practice, this makes it possible to update certificates
>>> without kernel update and reboot, aligning with module and kernel
>>> (kexec) signature verification which already use the secondary trusted
>>> keyring.
>>>
>>> Cc: Alasdair Kergon <[e-mail]>
>>> Cc: Andrew Morton <[e-mail]>
>>> Cc: Jarkko Sakkinen <[e-mail]>
>>> Cc: Jaskaran Khurana <[e-mail]>
>>> Cc: Mike Snitzer <[e-mail]>
>>> Cc: Milan Broz <[e-mail]>
>>> Signed-off-by: Mickaël Salaün <[e-mail]>
>>> ---
>>>
>>> Previous version:
>>> https://lore.kernel.org/lkml/[e-mail]/
>>>
>>> Changes since v1:
>>> * Extend the commit message (asked by Jarkko Sakkinen).
>>> * Rename the Kconfig "help" keyword according to commit 84af7a6194e4
>>>   ("checkpatch: kconfig: prefer 'help' over '---help---'").
>>
>> Can you please explain why you've decided to make this a Kconfig CONFIG
>> knob?  Why not either add: a dm-verity table argument? A dm-verity
>> kernel module parameter? or both (to allow a particular default but then
>> per-device override)?
> 
> The purpose of signed dm-verity images is to authenticate files, or said
> in another way, to enable the kernel to trust disk images in a flexible
> way (i.e. thanks to certificate's chain of trust). Being able to update
> such chain at run time requires to use the second trusted keyring. This
> keyring automatically includes the certificate authorities from the
> builtin trusted keyring, which are required to dynamically populate the
> secondary trusted keyring with certificates signed by an already trusted
> authority. The roots of trust must then be included at build time in the
> builtin trusted keyring.
> 
> To be meaningful, using dm-verity signatures implies to have a
> restricted user space, i.e. even the root user has limited power over
> the kernel and the rest of the system. Blindly trusting data provided by
> user space (e.g. dm-verity table argument, kernel module parameter)
> defeat the purpose of (mandatory) authenticated images.
> 
>>
>> Otherwise, _all_ DM verity devices will be configured to use secondary
>> keyring fallback.  Is that really desirable?
> 
> That is already the current state (on purpose).

I meant that when DM_VERITY_VERIFY_ROOTHASH_SIG is set, dm-verity
signature becomes mandatory. This new configuration
DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING extend this trust to the
secondary trusted keyring, which contains certificates signed (directly
or indirectly) by CA from the builtin trusted keyring.

So yes, this new (optional) configuration *extends* the source of trust
for all dm-verity devices, and yes, it is desirable. I think it should
have been this way from the beginning (as for other authentication
mechanisms) but it wasn't necessary at that time.

> 
>>
>> Regardless, I really don't see why a Kconfig knob is appropriate.
> 
> Moreover, a Kconfig knob makes sense as much as
> DM_VERITY_VERIFY_ROOTHASH_SIG,
> IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY, MODULE_SIG_FORCE and
> other similar authentication mechanisms. Indeed, when using these
> configurations, we want the kernel to enforce a specific policy.
> 
> Obviously, we can't make the DM_VERITY_VERIFY_ROOTHASH_SIG relies on the
> secondary trusted keyring without important security implications for
> systems already using this configuration (and then the builtin trusted
> keyring as the unique source of trust).
>

Milan Broz Oct. 16, 2020, 11:08 a.m. UTC | #4

On 16/10/2020 10:49, Mickaël Salaün wrote:
> On 16/10/2020 10:29, Mickaël Salaün wrote:
>>
>> On 15/10/2020 18:52, Mike Snitzer wrote:
>>> Can you please explain why you've decided to make this a Kconfig CONFIG
>>> knob?  Why not either add: a dm-verity table argument? A dm-verity
>>> kernel module parameter? or both (to allow a particular default but then
>>> per-device override)?
>>
>> The purpose of signed dm-verity images is to authenticate files, or said
>> in another way, to enable the kernel to trust disk images in a flexible
>> way (i.e. thanks to certificate's chain of trust). Being able to update
>> such chain at run time requires to use the second trusted keyring. This
>> keyring automatically includes the certificate authorities from the
>> builtin trusted keyring, which are required to dynamically populate the
>> secondary trusted keyring with certificates signed by an already trusted
>> authority. The roots of trust must then be included at build time in the
>> builtin trusted keyring.
>>
>> To be meaningful, using dm-verity signatures implies to have a
>> restricted user space, i.e. even the root user has limited power over
>> the kernel and the rest of the system. Blindly trusting data provided by
>> user space (e.g. dm-verity table argument, kernel module parameter)
>> defeat the purpose of (mandatory) authenticated images.
>>
>>>
>>> Otherwise, _all_ DM verity devices will be configured to use secondary
>>> keyring fallback.  Is that really desirable?
>>
>> That is already the current state (on purpose).
> 
> I meant that when DM_VERITY_VERIFY_ROOTHASH_SIG is set, dm-verity
> signature becomes mandatory. This new configuration
> DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING extend this trust to the
> secondary trusted keyring, which contains certificates signed (directly
> or indirectly) by CA from the builtin trusted keyring.
> 
> So yes, this new (optional) configuration *extends* the source of trust
> for all dm-verity devices, and yes, it is desirable. I think it should
> have been this way from the beginning (as for other authentication
> mechanisms) but it wasn't necessary at that time.

Well, I understand why you need a config option here.
And using the secondary keyring actually makes much more sense to me than
the original approach.

But please do not forget that dm-verity is sometimes used in different
contexts where such strict in-kernel certificate trust is unnecessary.
With your configure options set, you deliberately remove the possibility
to configure such devices.
I understand that it is needed for "trusted" systems, but we should be clear
in the documentation.
Maybe also add note to /Documentation/admin-guide/device-mapper/verity.rst ?
We already mention DM_VERITY_VERIFY_ROOTHASH_SIG there.

The current userspace configuration through veritysetup does not need
any patches for your patch, correct?

Thanks,
Milan

Mickaël Salaün Oct. 16, 2020, 12:19 p.m. UTC | #5

On 16/10/2020 13:08, Milan Broz wrote:
> On 16/10/2020 10:49, Mickaël Salaün wrote:
>> On 16/10/2020 10:29, Mickaël Salaün wrote:
>>>
>>> On 15/10/2020 18:52, Mike Snitzer wrote:
>>>> Can you please explain why you've decided to make this a Kconfig CONFIG
>>>> knob?  Why not either add: a dm-verity table argument? A dm-verity
>>>> kernel module parameter? or both (to allow a particular default but
>>>> then
>>>> per-device override)?
>>>
>>> The purpose of signed dm-verity images is to authenticate files, or said
>>> in another way, to enable the kernel to trust disk images in a flexible
>>> way (i.e. thanks to certificate's chain of trust). Being able to update
>>> such chain at run time requires to use the second trusted keyring. This
>>> keyring automatically includes the certificate authorities from the
>>> builtin trusted keyring, which are required to dynamically populate the
>>> secondary trusted keyring with certificates signed by an already trusted
>>> authority. The roots of trust must then be included at build time in the
>>> builtin trusted keyring.
>>>
>>> To be meaningful, using dm-verity signatures implies to have a
>>> restricted user space, i.e. even the root user has limited power over
>>> the kernel and the rest of the system. Blindly trusting data provided by
>>> user space (e.g. dm-verity table argument, kernel module parameter)
>>> defeat the purpose of (mandatory) authenticated images.
>>>
>>>>
>>>> Otherwise, _all_ DM verity devices will be configured to use secondary
>>>> keyring fallback.  Is that really desirable?
>>>
>>> That is already the current state (on purpose).
>>
>> I meant that when DM_VERITY_VERIFY_ROOTHASH_SIG is set, dm-verity
>> signature becomes mandatory. This new configuration
>> DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING extend this trust to the
>> secondary trusted keyring, which contains certificates signed (directly
>> or indirectly) by CA from the builtin trusted keyring.
>>
>> So yes, this new (optional) configuration *extends* the source of trust
>> for all dm-verity devices, and yes, it is desirable. I think it should
>> have been this way from the beginning (as for other authentication
>> mechanisms) but it wasn't necessary at that time.
> 
> Well, I understand why you need a config option here.
> And using the secondary keyring actually makes much more sense to me than
> the original approach.
> 
> But please do not forget that dm-verity is sometimes used in different
> contexts where such strict in-kernel certificate trust is unnecessary.
> With your configure options set, you deliberately remove the possibility
> to configure such devices.
It doesn't make sense to set DM_VERITY_VERIFY_ROOTHASH_SIG in generic
distro because such policy is configured at build time in the kernel
with hardcoded CAs. If the new option is not set then nothing change. I
don't see why it could be an issue for use cases we previously defined
(with DM_VERITY_VERIFY_ROOTHASH_SIG).

> I understand that it is needed for "trusted" systems, but we should be
> clear
> in the documentation.
> Maybe also add note to
> /Documentation/admin-guide/device-mapper/verity.rst ?
> We already mention DM_VERITY_VERIFY_ROOTHASH_SIG there.

The current documentation remains true.
DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING depends on
DM_VERITY_VERIFY_ROOTHASH_SIG.

> 
> The current userspace configuration through veritysetup does not need
> any patches for your patch, correct?

Right, it's only different from the kernel point of view.

> 
> Thanks,
> Milan
>

Mickaël Salaün Oct. 23, 2020, 10:20 a.m. UTC | #6

It seems that there is no more question. Mike, Alasdair, could you
please consider to merge this into the tree?

On 16/10/2020 14:19, Mickaël Salaün wrote:
> 
> On 16/10/2020 13:08, Milan Broz wrote:
>> On 16/10/2020 10:49, Mickaël Salaün wrote:
>>> On 16/10/2020 10:29, Mickaël Salaün wrote:
>>>>
>>>> On 15/10/2020 18:52, Mike Snitzer wrote:
>>>>> Can you please explain why you've decided to make this a Kconfig CONFIG
>>>>> knob?  Why not either add: a dm-verity table argument? A dm-verity
>>>>> kernel module parameter? or both (to allow a particular default but
>>>>> then
>>>>> per-device override)?
>>>>
>>>> The purpose of signed dm-verity images is to authenticate files, or said
>>>> in another way, to enable the kernel to trust disk images in a flexible
>>>> way (i.e. thanks to certificate's chain of trust). Being able to update
>>>> such chain at run time requires to use the second trusted keyring. This
>>>> keyring automatically includes the certificate authorities from the
>>>> builtin trusted keyring, which are required to dynamically populate the
>>>> secondary trusted keyring with certificates signed by an already trusted
>>>> authority. The roots of trust must then be included at build time in the
>>>> builtin trusted keyring.
>>>>
>>>> To be meaningful, using dm-verity signatures implies to have a
>>>> restricted user space, i.e. even the root user has limited power over
>>>> the kernel and the rest of the system. Blindly trusting data provided by
>>>> user space (e.g. dm-verity table argument, kernel module parameter)
>>>> defeat the purpose of (mandatory) authenticated images.
>>>>
>>>>>
>>>>> Otherwise, _all_ DM verity devices will be configured to use secondary
>>>>> keyring fallback.  Is that really desirable?
>>>>
>>>> That is already the current state (on purpose).
>>>
>>> I meant that when DM_VERITY_VERIFY_ROOTHASH_SIG is set, dm-verity
>>> signature becomes mandatory. This new configuration
>>> DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING extend this trust to the
>>> secondary trusted keyring, which contains certificates signed (directly
>>> or indirectly) by CA from the builtin trusted keyring.
>>>
>>> So yes, this new (optional) configuration *extends* the source of trust
>>> for all dm-verity devices, and yes, it is desirable. I think it should
>>> have been this way from the beginning (as for other authentication
>>> mechanisms) but it wasn't necessary at that time.
>>
>> Well, I understand why you need a config option here.
>> And using the secondary keyring actually makes much more sense to me than
>> the original approach.
>>
>> But please do not forget that dm-verity is sometimes used in different
>> contexts where such strict in-kernel certificate trust is unnecessary.
>> With your configure options set, you deliberately remove the possibility
>> to configure such devices.
> It doesn't make sense to set DM_VERITY_VERIFY_ROOTHASH_SIG in generic
> distro because such policy is configured at build time in the kernel
> with hardcoded CAs. If the new option is not set then nothing change. I
> don't see why it could be an issue for use cases we previously defined
> (with DM_VERITY_VERIFY_ROOTHASH_SIG).
> 
>> I understand that it is needed for "trusted" systems, but we should be
>> clear
>> in the documentation.
>> Maybe also add note to
>> /Documentation/admin-guide/device-mapper/verity.rst ?
>> We already mention DM_VERITY_VERIFY_ROOTHASH_SIG there.
> 
> The current documentation remains true.
> DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING depends on
> DM_VERITY_VERIFY_ROOTHASH_SIG.
> 
>>
>> The current userspace configuration through veritysetup does not need
>> any patches for your patch, correct?
> 
> Right, it's only different from the kernel point of view.
> 
>>
>> Thanks,
>> Milan
>>

Mike Snitzer Oct. 23, 2020, 3:11 p.m. UTC | #7

On Fri, Oct 23 2020 at  6:20am -0400,
Mickaël Salaün <[e-mail]> wrote:

> It seems that there is no more question. Mike, Alasdair, could you
> please consider to merge this into the tree?
> 
> On 16/10/2020 14:19, Mickaël Salaün wrote:
> > 
> > On 16/10/2020 13:08, Milan Broz wrote:
> >> On 16/10/2020 10:49, Mickaël Salaün wrote:
> >>> On 16/10/2020 10:29, Mickaël Salaün wrote:
> >>>>
> >>>> On 15/10/2020 18:52, Mike Snitzer wrote:
> >>>>> Can you please explain why you've decided to make this a Kconfig CONFIG
> >>>>> knob?  Why not either add: a dm-verity table argument? A dm-verity
> >>>>> kernel module parameter? or both (to allow a particular default but
> >>>>> then
> >>>>> per-device override)?
> >>>>
> >>>> The purpose of signed dm-verity images is to authenticate files, or said
> >>>> in another way, to enable the kernel to trust disk images in a flexible
> >>>> way (i.e. thanks to certificate's chain of trust). Being able to update
> >>>> such chain at run time requires to use the second trusted keyring. This
> >>>> keyring automatically includes the certificate authorities from the
> >>>> builtin trusted keyring, which are required to dynamically populate the
> >>>> secondary trusted keyring with certificates signed by an already trusted
> >>>> authority. The roots of trust must then be included at build time in the
> >>>> builtin trusted keyring.
> >>>>
> >>>> To be meaningful, using dm-verity signatures implies to have a
> >>>> restricted user space, i.e. even the root user has limited power over
> >>>> the kernel and the rest of the system. Blindly trusting data provided by
> >>>> user space (e.g. dm-verity table argument, kernel module parameter)
> >>>> defeat the purpose of (mandatory) authenticated images.
> >>>>
> >>>>>
> >>>>> Otherwise, _all_ DM verity devices will be configured to use secondary
> >>>>> keyring fallback.  Is that really desirable?
> >>>>
> >>>> That is already the current state (on purpose).
> >>>
> >>> I meant that when DM_VERITY_VERIFY_ROOTHASH_SIG is set, dm-verity
> >>> signature becomes mandatory. This new configuration
> >>> DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING extend this trust to the
> >>> secondary trusted keyring, which contains certificates signed (directly
> >>> or indirectly) by CA from the builtin trusted keyring.
> >>>
> >>> So yes, this new (optional) configuration *extends* the source of trust
> >>> for all dm-verity devices, and yes, it is desirable. I think it should
> >>> have been this way from the beginning (as for other authentication
> >>> mechanisms) but it wasn't necessary at that time.
> >>
> >> Well, I understand why you need a config option here.
> >> And using the secondary keyring actually makes much more sense to me than
> >> the original approach.
> >>
> >> But please do not forget that dm-verity is sometimes used in different
> >> contexts where such strict in-kernel certificate trust is unnecessary.
> >> With your configure options set, you deliberately remove the possibility
> >> to configure such devices.
> > It doesn't make sense to set DM_VERITY_VERIFY_ROOTHASH_SIG in generic
> > distro because such policy is configured at build time in the kernel
> > with hardcoded CAs. If the new option is not set then nothing change. I
> > don't see why it could be an issue for use cases we previously defined
> > (with DM_VERITY_VERIFY_ROOTHASH_SIG).
> > 
> >> I understand that it is needed for "trusted" systems, but we should be
> >> clear
> >> in the documentation.
> >> Maybe also add note to
> >> /Documentation/admin-guide/device-mapper/verity.rst ?
> >> We already mention DM_VERITY_VERIFY_ROOTHASH_SIG there.
> > 
> > The current documentation remains true.
> > DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING depends on
> > DM_VERITY_VERIFY_ROOTHASH_SIG.

Yes, while true that doesn't change the fact that documenting
DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING is useful to potential
consumers of baseline DM_VERITY_VERIFY_ROOTHASH_SIG.

Please update Documentation and post v3, I'll get it merged for 5.11.

Thanks,
Mike

Patch

diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index 30ba3573626c..1d68935e45ef 100644
--- a/drivers/md/Kconfig
+++ b/drivers/md/Kconfig
@@ -530,11 +530,22 @@  config DM_VERITY_VERIFY_ROOTHASH_SIG
 	bool "Verity data device root hash signature verification support"
 	depends on DM_VERITY
 	select SYSTEM_DATA_VERIFICATION
-	  help
+	help
 	  Add ability for dm-verity device to be validated if the
 	  pre-generated tree of cryptographic checksums passed has a pkcs#7
 	  signature file that can validate the roothash of the tree.
 
+	  By default, rely on the builtin trusted keyring.
+
+	  If unsure, say N.
+
+config DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING
+	bool "Verity data device root hash signature verification with secondary keyring"
+	depends on DM_VERITY_VERIFY_ROOTHASH_SIG
+	depends on SECONDARY_TRUSTED_KEYRING
+	help
+	  Rely on the secondary trusted keyring to verify dm-verity signatures.
+
 	  If unsure, say N.
 
 config DM_VERITY_FEC
diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
index 614e43db93aa..29385dc470d5 100644
--- a/drivers/md/dm-verity-verify-sig.c
+++ b/drivers/md/dm-verity-verify-sig.c
@@ -119,8 +119,13 @@  int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
 	}
 
 	ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
-				sig_len, NULL, VERIFYING_UNSPECIFIED_SIGNATURE,
-				NULL, NULL);
+				sig_len,
+#ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING
+				VERIFY_USE_SECONDARY_KEYRING,
+#else
+				NULL,
+#endif
+				VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
 
 	return ret;
 }