diff mbox series

[v8,09/17] KEYS: Rename get_builtin_and_secondary_restriction

Message ID 20211124044124.998170-10-eric.snowberg@oracle.com (mailing list archive)
State New
Headers show
Series Enroll kernel keys thru MOK | expand

Commit Message

Eric Snowberg Nov. 24, 2021, 4:41 a.m. UTC
In preparation for returning either the existing
restrict_link_by_builtin_and_secondary_trusted or the upcoming
restriction that includes the trusted builtin, secondary and
machine keys, to improve clarity, rename
get_builtin_and_secondary_restriction to get_secondary_restriction.

Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
v6: Initial version
v7: Unmodified from v7
v8: Code unmodified from v7, added Mimi's Reviewed-by
---
 certs/system_keyring.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jarkko Sakkinen Nov. 27, 2021, 12:49 a.m. UTC | #1
On Tue, 2021-11-23 at 23:41 -0500, Eric Snowberg wrote:
> In preparation for returning either the existing
> restrict_link_by_builtin_and_secondary_trusted or the upcoming
> restriction that includes the trusted builtin, secondary and
> machine keys, to improve clarity, rename
> get_builtin_and_secondary_restriction to get_secondary_restriction.
> 
> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
> v6: Initial version
> v7: Unmodified from v7
> v8: Code unmodified from v7, added Mimi's Reviewed-by
> ---
>  certs/system_keyring.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 692365dee2bd..8f1f87579819 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -77,7 +77,7 @@ int restrict_link_by_builtin_and_secondary_trusted(
>   * Allocate a struct key_restriction for the "builtin and secondary trust"
>   * keyring. Only for use in system_trusted_keyring_init().
>   */
> -static __init struct key_restriction *get_builtin_and_secondary_restriction(void)
> +static __init struct key_restriction *get_secondary_restriction(void)
>  {
>         struct key_restriction *restriction;
>  
> @@ -117,7 +117,7 @@ static __init int system_trusted_keyring_init(void)
>                                KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH |
>                                KEY_USR_WRITE),
>                               KEY_ALLOC_NOT_IN_QUOTA,
> -                             get_builtin_and_secondary_restriction(),
> +                             get_secondary_restriction(),
>                               NULL);
>         if (IS_ERR(secondary_trusted_keys))
>                 panic("Can't allocate secondary trusted keyring\n");

This is wrong order.

You should first do the changes that make the old name
obsolete and only after that have a patch that does the
rename. Unfortunately, this patch cannot possibly acked
with the current order.


/Jarkko
Eric Snowberg Nov. 30, 2021, 5:21 p.m. UTC | #2
> On Nov 26, 2021, at 5:49 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Tue, 2021-11-23 at 23:41 -0500, Eric Snowberg wrote:
>> In preparation for returning either the existing
>> restrict_link_by_builtin_and_secondary_trusted or the upcoming
>> restriction that includes the trusted builtin, secondary and
>> machine keys, to improve clarity, rename
>> get_builtin_and_secondary_restriction to get_secondary_restriction.
>> 
>> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
>> ---
>> v6: Initial version
>> v7: Unmodified from v7
>> v8: Code unmodified from v7, added Mimi's Reviewed-by
>> ---
>>  certs/system_keyring.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
>> index 692365dee2bd..8f1f87579819 100644
>> --- a/certs/system_keyring.c
>> +++ b/certs/system_keyring.c
>> @@ -77,7 +77,7 @@ int restrict_link_by_builtin_and_secondary_trusted(
>>   * Allocate a struct key_restriction for the "builtin and secondary trust"
>>   * keyring. Only for use in system_trusted_keyring_init().
>>   */
>> -static __init struct key_restriction *get_builtin_and_secondary_restriction(void)
>> +static __init struct key_restriction *get_secondary_restriction(void)
>>  {
>>         struct key_restriction *restriction;
>>  
>> @@ -117,7 +117,7 @@ static __init int system_trusted_keyring_init(void)
>>                                KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH |
>>                                KEY_USR_WRITE),
>>                               KEY_ALLOC_NOT_IN_QUOTA,
>> -                             get_builtin_and_secondary_restriction(),
>> +                             get_secondary_restriction(),
>>                               NULL);
>>         if (IS_ERR(secondary_trusted_keys))
>>                 panic("Can't allocate secondary trusted keyring\n");
> 
> This is wrong order.
> 
> You should first do the changes that make the old name
> obsolete and only after that have a patch that does the
> rename. Unfortunately, this patch cannot possibly acked
> with the current order.

I can change the order, but I'm confused how this would work for a git bisect. 
If the rename happens afterwards, now two patches will always need to be 
reverted instead of the possibility of one.  Is this your expectation?
Jarkko Sakkinen Dec. 1, 2021, 10:27 a.m. UTC | #3
On Tue, Nov 30, 2021 at 05:21:45PM +0000, Eric Snowberg wrote:
> 
> 
> > On Nov 26, 2021, at 5:49 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > 
> > On Tue, 2021-11-23 at 23:41 -0500, Eric Snowberg wrote:
> >> In preparation for returning either the existing
> >> restrict_link_by_builtin_and_secondary_trusted or the upcoming
> >> restriction that includes the trusted builtin, secondary and
> >> machine keys, to improve clarity, rename
> >> get_builtin_and_secondary_restriction to get_secondary_restriction.
> >> 
> >> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> >> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> >> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> >> ---
> >> v6: Initial version
> >> v7: Unmodified from v7
> >> v8: Code unmodified from v7, added Mimi's Reviewed-by
> >> ---
> >>  certs/system_keyring.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> >> index 692365dee2bd..8f1f87579819 100644
> >> --- a/certs/system_keyring.c
> >> +++ b/certs/system_keyring.c
> >> @@ -77,7 +77,7 @@ int restrict_link_by_builtin_and_secondary_trusted(
> >>   * Allocate a struct key_restriction for the "builtin and secondary trust"
> >>   * keyring. Only for use in system_trusted_keyring_init().
> >>   */
> >> -static __init struct key_restriction *get_builtin_and_secondary_restriction(void)
> >> +static __init struct key_restriction *get_secondary_restriction(void)
> >>  {
> >>         struct key_restriction *restriction;
> >>  
> >> @@ -117,7 +117,7 @@ static __init int system_trusted_keyring_init(void)
> >>                                KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH |
> >>                                KEY_USR_WRITE),
> >>                               KEY_ALLOC_NOT_IN_QUOTA,
> >> -                             get_builtin_and_secondary_restriction(),
> >> +                             get_secondary_restriction(),
> >>                               NULL);
> >>         if (IS_ERR(secondary_trusted_keys))
> >>                 panic("Can't allocate secondary trusted keyring\n");
> > 
> > This is wrong order.
> > 
> > You should first do the changes that make the old name
> > obsolete and only after that have a patch that does the
> > rename. Unfortunately, this patch cannot possibly acked
> > with the current order.
> 
> I can change the order, but I'm confused how this would work for a git bisect. 
> If the rename happens afterwards, now two patches will always need to be 
> reverted instead of the possibility of one.  Is this your expectation?

I'd drop this patch altogether. Old name is a bit ugly but does it matter
all that much?

You already 16 patches without this.

/Jarkko
Mimi Zohar Dec. 1, 2021, 1:46 p.m. UTC | #4
On Wed, 2021-12-01 at 12:27 +0200, Jarkko Sakkinen wrote:
> On Tue, Nov 30, 2021 at 05:21:45PM +0000, Eric Snowberg wrote:
> > 
> > 
> > > On Nov 26, 2021, at 5:49 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > 
> > > On Tue, 2021-11-23 at 23:41 -0500, Eric Snowberg wrote:
> > >> In preparation for returning either the existing
> > >> restrict_link_by_builtin_and_secondary_trusted or the upcoming
> > >> restriction that includes the trusted builtin, secondary and
> > >> machine keys, to improve clarity, rename
> > >> get_builtin_and_secondary_restriction to get_secondary_restriction.
> > >> 
> > >> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> > >> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> > >> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > >> ---
> > >> v6: Initial version
> > >> v7: Unmodified from v7
> > >> v8: Code unmodified from v7, added Mimi's Reviewed-by
> > >> ---
> > >>  certs/system_keyring.c | 4 ++--
> > >>  1 file changed, 2 insertions(+), 2 deletions(-)
> > >> 
> > >> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> > >> index 692365dee2bd..8f1f87579819 100644
> > >> --- a/certs/system_keyring.c
> > >> +++ b/certs/system_keyring.c
> > >> @@ -77,7 +77,7 @@ int restrict_link_by_builtin_and_secondary_trusted(
> > >>   * Allocate a struct key_restriction for the "builtin and secondary trust"
> > >>   * keyring. Only for use in system_trusted_keyring_init().
> > >>   */
> > >> -static __init struct key_restriction *get_builtin_and_secondary_restriction(void)
> > >> +static __init struct key_restriction *get_secondary_restriction(void)
> > >>  {
> > >>         struct key_restriction *restriction;
> > >>  
> > >> @@ -117,7 +117,7 @@ static __init int system_trusted_keyring_init(void)
> > >>                                KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH |
> > >>                                KEY_USR_WRITE),
> > >>                               KEY_ALLOC_NOT_IN_QUOTA,
> > >> -                             get_builtin_and_secondary_restriction(),
> > >> +                             get_secondary_restriction(),
> > >>                               NULL);
> > >>         if (IS_ERR(secondary_trusted_keys))
> > >>                 panic("Can't allocate secondary trusted keyring\n");
> > > 
> > > This is wrong order.
> > > 
> > > You should first do the changes that make the old name
> > > obsolete and only after that have a patch that does the
> > > rename. Unfortunately, this patch cannot possibly acked
> > > with the current order.
> >
> > I can change the order, but I'm confused how this would work for a git bisect. 
> > If the rename happens afterwards, now two patches will always need to be 
> > reverted instead of the possibility of one.  Is this your expectation?

If the keyring name change is independent of any other changes, as
Jarkko suggested, nothing would break.

> I'd drop this patch altogether. Old name is a bit ugly but does it matter
> all that much?

The name "get_builtin_and_secondary_restriction" implies trust based on
keys in the ".builtin_trusted_keys" and ".secondary_trusted_keys"
keyrings.  This patch set is extending that to include keys on the new
".machine" keyring, by linking it to the secondary keyring.  Is leaving
the name unchanged really an option?

> 
> You already 16 patches without this.

Agreed, it's a lot.  In the past, I've asked Eric to see if some of
them could be squashed.

Mimi
Jarkko Sakkinen Dec. 4, 2021, 5:39 p.m. UTC | #5
On Wed, Dec 01, 2021 at 08:46:53AM -0500, Mimi Zohar wrote:
> On Wed, 2021-12-01 at 12:27 +0200, Jarkko Sakkinen wrote:
> > On Tue, Nov 30, 2021 at 05:21:45PM +0000, Eric Snowberg wrote:
> > > 
> > > 
> > > > On Nov 26, 2021, at 5:49 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > > 
> > > > On Tue, 2021-11-23 at 23:41 -0500, Eric Snowberg wrote:
> > > >> In preparation for returning either the existing
> > > >> restrict_link_by_builtin_and_secondary_trusted or the upcoming
> > > >> restriction that includes the trusted builtin, secondary and
> > > >> machine keys, to improve clarity, rename
> > > >> get_builtin_and_secondary_restriction to get_secondary_restriction.
> > > >> 
> > > >> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> > > >> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> > > >> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > > >> ---
> > > >> v6: Initial version
> > > >> v7: Unmodified from v7
> > > >> v8: Code unmodified from v7, added Mimi's Reviewed-by
> > > >> ---
> > > >>  certs/system_keyring.c | 4 ++--
> > > >>  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >> 
> > > >> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> > > >> index 692365dee2bd..8f1f87579819 100644
> > > >> --- a/certs/system_keyring.c
> > > >> +++ b/certs/system_keyring.c
> > > >> @@ -77,7 +77,7 @@ int restrict_link_by_builtin_and_secondary_trusted(
> > > >>   * Allocate a struct key_restriction for the "builtin and secondary trust"
> > > >>   * keyring. Only for use in system_trusted_keyring_init().
> > > >>   */
> > > >> -static __init struct key_restriction *get_builtin_and_secondary_restriction(void)
> > > >> +static __init struct key_restriction *get_secondary_restriction(void)
> > > >>  {
> > > >>         struct key_restriction *restriction;
> > > >>  
> > > >> @@ -117,7 +117,7 @@ static __init int system_trusted_keyring_init(void)
> > > >>                                KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH |
> > > >>                                KEY_USR_WRITE),
> > > >>                               KEY_ALLOC_NOT_IN_QUOTA,
> > > >> -                             get_builtin_and_secondary_restriction(),
> > > >> +                             get_secondary_restriction(),
> > > >>                               NULL);
> > > >>         if (IS_ERR(secondary_trusted_keys))
> > > >>                 panic("Can't allocate secondary trusted keyring\n");
> > > > 
> > > > This is wrong order.
> > > > 
> > > > You should first do the changes that make the old name
> > > > obsolete and only after that have a patch that does the
> > > > rename. Unfortunately, this patch cannot possibly acked
> > > > with the current order.
> > >
> > > I can change the order, but I'm confused how this would work for a git bisect. 
> > > If the rename happens afterwards, now two patches will always need to be 
> > > reverted instead of the possibility of one.  Is this your expectation?
> 
> If the keyring name change is independent of any other changes, as
> Jarkko suggested, nothing would break.
> 
> > I'd drop this patch altogether. Old name is a bit ugly but does it matter
> > all that much?
> 
> The name "get_builtin_and_secondary_restriction" implies trust based on
> keys in the ".builtin_trusted_keys" and ".secondary_trusted_keys"
> keyrings.  This patch set is extending that to include keys on the new
> ".machine" keyring, by linking it to the secondary keyring.  Is leaving
> the name unchanged really an option?

Yes, it is an option, as long as it is documented correctly in the
prepending kdoc the symbol name does not matter all that much..

/Jarkko
Eric Snowberg Dec. 15, 2021, 6:14 p.m. UTC | #6
> On Dec 1, 2021, at 6:46 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Wed, 2021-12-01 at 12:27 +0200, Jarkko Sakkinen wrote:
>> On Tue, Nov 30, 2021 at 05:21:45PM +0000, Eric Snowberg wrote:
>>> 
>>> 
>>>> On Nov 26, 2021, at 5:49 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
>>>> 
>>>> On Tue, 2021-11-23 at 23:41 -0500, Eric Snowberg wrote:
>>>>> In preparation for returning either the existing
>>>>> restrict_link_by_builtin_and_secondary_trusted or the upcoming
>>>>> restriction that includes the trusted builtin, secondary and
>>>>> machine keys, to improve clarity, rename
>>>>> get_builtin_and_secondary_restriction to get_secondary_restriction.
>>>>> 
>>>>> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
>>>>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>>>>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
>>>>> ---
>>>>> v6: Initial version
>>>>> v7: Unmodified from v7
>>>>> v8: Code unmodified from v7, added Mimi's Reviewed-by
>>>>> ---
>>>>> certs/system_keyring.c | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>> 
>>>>> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
>>>>> index 692365dee2bd..8f1f87579819 100644
>>>>> --- a/certs/system_keyring.c
>>>>> +++ b/certs/system_keyring.c
>>>>> @@ -77,7 +77,7 @@ int restrict_link_by_builtin_and_secondary_trusted(
>>>>>  * Allocate a struct key_restriction for the "builtin and secondary trust"
>>>>>  * keyring. Only for use in system_trusted_keyring_init().
>>>>>  */
>>>>> -static __init struct key_restriction *get_builtin_and_secondary_restriction(void)
>>>>> +static __init struct key_restriction *get_secondary_restriction(void)
>>>>> {
>>>>>        struct key_restriction *restriction;
>>>>> 
>>>>> @@ -117,7 +117,7 @@ static __init int system_trusted_keyring_init(void)
>>>>>                               KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH |
>>>>>                               KEY_USR_WRITE),
>>>>>                              KEY_ALLOC_NOT_IN_QUOTA,
>>>>> -                             get_builtin_and_secondary_restriction(),
>>>>> +                             get_secondary_restriction(),
>>>>>                              NULL);
>>>>>        if (IS_ERR(secondary_trusted_keys))
>>>>>                panic("Can't allocate secondary trusted keyring\n");
>>>> 
>>>> This is wrong order.
>>>> 
>>>> You should first do the changes that make the old name
>>>> obsolete and only after that have a patch that does the
>>>> rename. Unfortunately, this patch cannot possibly acked
>>>> with the current order.
>>> 
>>> I can change the order, but I'm confused how this would work for a git bisect. 
>>> If the rename happens afterwards, now two patches will always need to be 
>>> reverted instead of the possibility of one.  Is this your expectation?
> 
> If the keyring name change is independent of any other changes, as
> Jarkko suggested, nothing would break.
> 
>> I'd drop this patch altogether. Old name is a bit ugly but does it matter
>> all that much?
> 
> The name "get_builtin_and_secondary_restriction" implies trust based on
> keys in the ".builtin_trusted_keys" and ".secondary_trusted_keys"
> keyrings.  This patch set is extending that to include keys on the new
> ".machine" keyring, by linking it to the secondary keyring.  Is leaving
> the name unchanged really an option?
> 
>> 
>> You already 16 patches without this.
> 
> Agreed, it's a lot.  In the past, I've asked Eric to see if some of
> them could be squashed.

The series grew based on requests added in each round.  How about
I drop IMA support from the next round?  This would eliminate nine patches
from the series (5, 6, 7, 9-14), leaving six patches to introduce and enable 
the new machine keyring (3, 4, 8, 15-17).  The first two patches could 
be taken today.  The only reason the first two are included is to get this series 
through the kernel test robot.

This would allow the machine keyring to be used for module signing.  Afterwards 
I could introduce the CA restriction behind another Kconfig in a different series to 
add back IMA support. Let me know if this would be a better approach.
Mimi Zohar Dec. 15, 2021, 7:54 p.m. UTC | #7
On Wed, 2021-12-15 at 18:14 +0000, Eric Snowberg wrote:
> 
> > On Dec 1, 2021, at 6:46 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > On Wed, 2021-12-01 at 12:27 +0200, Jarkko Sakkinen wrote:
> >> On Tue, Nov 30, 2021 at 05:21:45PM +0000, Eric Snowberg wrote:
> >>> 
> >>> 
> >>>> On Nov 26, 2021, at 5:49 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >>>> 
> >>>> On Tue, 2021-11-23 at 23:41 -0500, Eric Snowberg wrote:
> >>>>> In preparation for returning either the existing
> >>>>> restrict_link_by_builtin_and_secondary_trusted or the upcoming
> >>>>> restriction that includes the trusted builtin, secondary and
> >>>>> machine keys, to improve clarity, rename
> >>>>> get_builtin_and_secondary_restriction to get_secondary_restriction.
> >>>>> 
> >>>>> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> >>>>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> >>>>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> >>>>> ---
> >>>>> v6: Initial version
> >>>>> v7: Unmodified from v7
> >>>>> v8: Code unmodified from v7, added Mimi's Reviewed-by
> >>>>> ---
> >>>>> certs/system_keyring.c | 4 ++--
> >>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>> 
> >>>>> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> >>>>> index 692365dee2bd..8f1f87579819 100644
> >>>>> --- a/certs/system_keyring.c
> >>>>> +++ b/certs/system_keyring.c
> >>>>> @@ -77,7 +77,7 @@ int restrict_link_by_builtin_and_secondary_trusted(
> >>>>>  * Allocate a struct key_restriction for the "builtin and secondary trust"
> >>>>>  * keyring. Only for use in system_trusted_keyring_init().
> >>>>>  */
> >>>>> -static __init struct key_restriction *get_builtin_and_secondary_restriction(void)
> >>>>> +static __init struct key_restriction *get_secondary_restriction(void)
> >>>>> {
> >>>>>        struct key_restriction *restriction;
> >>>>> 
> >>>>> @@ -117,7 +117,7 @@ static __init int system_trusted_keyring_init(void)
> >>>>>                               KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH |
> >>>>>                               KEY_USR_WRITE),
> >>>>>                              KEY_ALLOC_NOT_IN_QUOTA,
> >>>>> -                             get_builtin_and_secondary_restriction(),
> >>>>> +                             get_secondary_restriction(),
> >>>>>                              NULL);
> >>>>>        if (IS_ERR(secondary_trusted_keys))
> >>>>>                panic("Can't allocate secondary trusted keyring\n");
> >>>> 
> >>>> This is wrong order.
> >>>> 
> >>>> You should first do the changes that make the old name
> >>>> obsolete and only after that have a patch that does the
> >>>> rename. Unfortunately, this patch cannot possibly acked
> >>>> with the current order.
> >>> 
> >>> I can change the order, but I'm confused how this would work for a git bisect. 
> >>> If the rename happens afterwards, now two patches will always need to be 
> >>> reverted instead of the possibility of one.  Is this your expectation?
> > 
> > If the keyring name change is independent of any other changes, as
> > Jarkko suggested, nothing would break.
> > 
> >> I'd drop this patch altogether. Old name is a bit ugly but does it matter
> >> all that much?
> > 
> > The name "get_builtin_and_secondary_restriction" implies trust based on
> > keys in the ".builtin_trusted_keys" and ".secondary_trusted_keys"
> > keyrings.  This patch set is extending that to include keys on the new
> > ".machine" keyring, by linking it to the secondary keyring.  Is leaving
> > the name unchanged really an option?
> > 
> >> 
> >> You already 16 patches without this.
> > 
> > Agreed, it's a lot.  In the past, I've asked Eric to see if some of
> > them could be squashed.
> 
> The series grew based on requests added in each round.  How about
> I drop IMA support from the next round?  This would eliminate nine patches
> from the series (5, 6, 7, 9-14), leaving six patches to introduce and enable 
> the new machine keyring (3, 4, 8, 15-17).  The first two patches could 
> be taken today.  The only reason the first two are included is to get this series 
> through the kernel test robot.
> 
> This would allow the machine keyring to be used for module signing.  Afterwards 
> I could introduce the CA restriction behind another Kconfig in a different series to 
> add back IMA support. Let me know if this would be a better approach.

Once you allow ALL the MOK keys to be loaded onto the .machine keyring,
limiting which keys should be loaded will change the existing expected
behavior.  So you indeed would need to define a new config option.

Your patch set links the ".machine" keyring to the
".secondary_trusted_keys" kerying. Based on
IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY config being
enabled, determines whether IMA trusts the secondary keyring.

From IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY:
        help
          Keys may be added to the IMA or IMA blacklist keyrings, if
the
          key is validly signed by a CA cert in the system built-in or
          secondary trusted keyrings.

          Intermediate keys between those the kernel has compiled in
and the
          IMA keys to be added may be added to the system secondary
keyring,
          provided they are validly signed by a key already resident in
the
          built-in or secondary trusted keyrings.

A dependency to prevent enabling this configuration, if the ".machine"
keyring is enabled, would need to be defined.

thanks,

Mimi
diff mbox series

Patch

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 692365dee2bd..8f1f87579819 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -77,7 +77,7 @@  int restrict_link_by_builtin_and_secondary_trusted(
  * Allocate a struct key_restriction for the "builtin and secondary trust"
  * keyring. Only for use in system_trusted_keyring_init().
  */
-static __init struct key_restriction *get_builtin_and_secondary_restriction(void)
+static __init struct key_restriction *get_secondary_restriction(void)
 {
 	struct key_restriction *restriction;
 
@@ -117,7 +117,7 @@  static __init int system_trusted_keyring_init(void)
 			       KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH |
 			       KEY_USR_WRITE),
 			      KEY_ALLOC_NOT_IN_QUOTA,
-			      get_builtin_and_secondary_restriction(),
+			      get_secondary_restriction(),
 			      NULL);
 	if (IS_ERR(secondary_trusted_keys))
 		panic("Can't allocate secondary trusted keyring\n");