| Message ID | 20220321174548.510516-2-mic@digikod.net (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Explain panic() calls for keyring initialization | expand |
On Mon, Mar 21, 2022 at 1:45 PM Mickaël Salaün <mic@digikod.net> wrote: > > From: Mickaël Salaün <mic@linux.microsoft.com> > > The blacklist_init() function calls panic() for memory allocation > errors. This change documents the reason why we don't return -ENODEV. > > Suggested-by: Paul Moore <paul@paul-moore.com> [1] > Requested-by: Jarkko Sakkinen <jarkko@kernel.org> [1] > Link: https://lore.kernel.org/r/YjeW2r6Wv55Du0bJ@iki.fi [1] > Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com> > Link: https://lore.kernel.org/r/20220321174548.510516-2-mic@digikod.net > --- > certs/blacklist.c | 8 ++++++++ > 1 file changed, 8 insertions(+) I would suggest changing the second sentence as shown below, but otherwise it looks good to me. Reviewed-by: Paul Moore <paul@paul-moore.com> > diff --git a/certs/blacklist.c b/certs/blacklist.c > index 486ce0dd8e9c..ac26bcf9b9a5 100644 > --- a/certs/blacklist.c > +++ b/certs/blacklist.c > @@ -307,6 +307,14 @@ static int restrict_link_for_blacklist(struct key *dest_keyring, > > /* > * Initialise the blacklist > + * > + * The blacklist_init() function is registered as an initcall via > + * device_initcall(). As a result the functionality doesn't load and the "As a result if the blacklist_init() function fails for any reason the kernel continues to execute." > + * kernel continues on executing. While cleanly returning -ENODEV could be > + * acceptable for some non-critical kernel parts, if the blacklist keyring > + * fails to load it defeats the certificate/key based deny list for signed > + * modules. If a critical piece of security functionality that users expect to > + * be present fails to initialize, panic()ing is likely the right thing to do. > */ > static int __init blacklist_init(void) > { -- paul-moore.com
On Mon, Mar 21, 2022 at 02:23:54PM -0400, Paul Moore wrote: > On Mon, Mar 21, 2022 at 1:45 PM Mickaël Salaün <mic@digikod.net> wrote: > > > > From: Mickaël Salaün <mic@linux.microsoft.com> > > > > The blacklist_init() function calls panic() for memory allocation > > errors. This change documents the reason why we don't return -ENODEV. > > > > Suggested-by: Paul Moore <paul@paul-moore.com> [1] > > Requested-by: Jarkko Sakkinen <jarkko@kernel.org> [1] > > Link: https://lore.kernel.org/r/YjeW2r6Wv55Du0bJ@iki.fi [1] > > Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com> > > Link: https://lore.kernel.org/r/20220321174548.510516-2-mic@digikod.net > > --- > > certs/blacklist.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > I would suggest changing the second sentence as shown below, but > otherwise it looks good to me. > > Reviewed-by: Paul Moore <paul@paul-moore.com> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> Mickaël, I think since your patch set was not huge in the first place, I'm considering making it part of rc2 pull request while I normally try to avoid any features after rc1. It's anyway throughly tested, and generally has been around for a *long time*. I've even tested it myself a few times. Just trying to be responsible as a maintainer and if something does not feel right, I don't try to pretend that "I get it", if you know what I mean. This fully clarifies "not getting it" part :-) Thanks! BR, Jarkko
On 21/03/2022 19:23, Paul Moore wrote: > On Mon, Mar 21, 2022 at 1:45 PM Mickaël Salaün <mic@digikod.net> wrote: >> >> From: Mickaël Salaün <mic@linux.microsoft.com> >> >> The blacklist_init() function calls panic() for memory allocation >> errors. This change documents the reason why we don't return -ENODEV. >> >> Suggested-by: Paul Moore <paul@paul-moore.com> [1] >> Requested-by: Jarkko Sakkinen <jarkko@kernel.org> [1] >> Link: https://lore.kernel.org/r/YjeW2r6Wv55Du0bJ@iki.fi [1] >> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com> >> Link: https://lore.kernel.org/r/20220321174548.510516-2-mic@digikod.net >> --- >> certs/blacklist.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) > > I would suggest changing the second sentence as shown below, but > otherwise it looks good to me. > > Reviewed-by: Paul Moore <paul@paul-moore.com> > >> diff --git a/certs/blacklist.c b/certs/blacklist.c >> index 486ce0dd8e9c..ac26bcf9b9a5 100644 >> --- a/certs/blacklist.c >> +++ b/certs/blacklist.c >> @@ -307,6 +307,14 @@ static int restrict_link_for_blacklist(struct key *dest_keyring, >> >> /* >> * Initialise the blacklist >> + * >> + * The blacklist_init() function is registered as an initcall via >> + * device_initcall(). As a result the functionality doesn't load and the > > "As a result if the blacklist_init() function fails for any reason the > kernel continues to execute." Thanks, I'll fix that. > >> + * kernel continues on executing. While cleanly returning -ENODEV could be >> + * acceptable for some non-critical kernel parts, if the blacklist keyring >> + * fails to load it defeats the certificate/key based deny list for signed >> + * modules. If a critical piece of security functionality that users expect to >> + * be present fails to initialize, panic()ing is likely the right thing to do. >> */ >> static int __init blacklist_init(void) >> { > > -- > paul-moore.com
On 22/03/2022 00:53, Jarkko Sakkinen wrote: > On Mon, Mar 21, 2022 at 02:23:54PM -0400, Paul Moore wrote: >> On Mon, Mar 21, 2022 at 1:45 PM Mickaël Salaün <mic@digikod.net> wrote: >>> >>> From: Mickaël Salaün <mic@linux.microsoft.com> >>> >>> The blacklist_init() function calls panic() for memory allocation >>> errors. This change documents the reason why we don't return -ENODEV. >>> >>> Suggested-by: Paul Moore <paul@paul-moore.com> [1] >>> Requested-by: Jarkko Sakkinen <jarkko@kernel.org> [1] >>> Link: https://lore.kernel.org/r/YjeW2r6Wv55Du0bJ@iki.fi [1] >>> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com> >>> Link: https://lore.kernel.org/r/20220321174548.510516-2-mic@digikod.net >>> --- >>> certs/blacklist.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >> >> I would suggest changing the second sentence as shown below, but >> otherwise it looks good to me. >> >> Reviewed-by: Paul Moore <paul@paul-moore.com> > > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> > > Mickaël, I think since your patch set was not huge in the first place, I'm > considering making it part of rc2 pull request while I normally try to > avoid any features after rc1. It's anyway throughly tested, and generally > has been around for a *long time*. I've even tested it myself a few times. > > Just trying to be responsible as a maintainer and if something does not > feel right, I don't try to pretend that "I get it", if you know what > I mean. This fully clarifies "not getting it" part :-) > > Thanks! Thanks Jarkko, I get it. ;)
diff --git a/certs/blacklist.c b/certs/blacklist.c index 486ce0dd8e9c..ac26bcf9b9a5 100644 --- a/certs/blacklist.c +++ b/certs/blacklist.c @@ -307,6 +307,14 @@ static int restrict_link_for_blacklist(struct key *dest_keyring, /* * Initialise the blacklist + * + * The blacklist_init() function is registered as an initcall via + * device_initcall(). As a result the functionality doesn't load and the + * kernel continues on executing. While cleanly returning -ENODEV could be + * acceptable for some non-critical kernel parts, if the blacklist keyring + * fails to load it defeats the certificate/key based deny list for signed + * modules. If a critical piece of security functionality that users expect to + * be present fails to initialize, panic()ing is likely the right thing to do. */ static int __init blacklist_init(void) {