| Message ID | 20210510144054.48288-1-dimitri.ledkov@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [UPSTREAM,RFC] integrity: add informational messages when revoking certs. | expand |
On 10/05/2021 10:40, Dimitri John Ledkov wrote: > integrity_load_cert() prints messages of the source and cert details > when adding certs as trusted. Mirror those messages in > uefi_revocation_list_x509() when adding certs as revoked. > > Sample dmesg with this change: > > [ 1.538741] integrity: Platform Keyring initialized > [ 1.960071] integrity: Loading X.509 certificate: UEFI:db > [ 1.961986] integrity: Loaded X.509 cert 'Microsoft Corporation UEFI CA 2011: 13adbf4309bd82709c8cd54f316ed522988a1bd4' > [ 1.974041] integrity: Revoking X.509 certificate: UEFI:MokListXRT (MOKvar table) > [ 1.978852] blacklist: Revoked X.509 cert 'Canonical Ltd. Secure Boot Signing: 61482aa2830d0ab2ad5af10b7250da9033ddcef0' > [ 1.985850] integrity: Loading X.509 certificate: UEFI:MokListRT (MOKvar table) > [ 1.989651] integrity: Loaded X.509 cert 'Canonical Ltd. Master Certificate Authority: ad91990bc22ab1f517048c23b6655a268e345a63' Strip the [] from the dmesg. They make it harder to read. > > Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@canonical.com> > --- > > Another RFC patch that I'd like to submit upstream, based on > v5.13-rc1. I think you can just send it upstream and get the comments there. It's not confidential (already made public through kernel-team list) and upstream (specialized) folks might know this pieces better than us. > > certs/blacklist.c | 4 ++++ > security/integrity/platform_certs/keyring_handler.c | 1 + > 2 files changed, 5 insertions(+) > > diff --git a/certs/blacklist.c b/certs/blacklist.c > index c9a435b15af40..738c496756516 100644 > --- a/certs/blacklist.c > +++ b/certs/blacklist.c > @@ -172,6 +172,10 @@ int add_key_to_revocation_list(const char *data, size_t size) > if (IS_ERR(key)) { > pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key)); > return PTR_ERR(key); > + } else { > + pr_notice("Revoked X.509 cert '%s'\n", > + key_ref_to_ptr(key)->description); > + key_ref_put(key); This seems unrelated, based on the commit description. Why do you need it? Best regards, Krzysztof
On Mon, May 10, 2021 at 3:48 PM Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 10/05/2021 10:40, Dimitri John Ledkov wrote: > > integrity_load_cert() prints messages of the source and cert details > > when adding certs as trusted. Mirror those messages in > > uefi_revocation_list_x509() when adding certs as revoked. > > > > Sample dmesg with this change: > > > > [ 1.538741] integrity: Platform Keyring initialized > > [ 1.960071] integrity: Loading X.509 certificate: UEFI:db > > [ 1.961986] integrity: Loaded X.509 cert 'Microsoft Corporation UEFI CA 2011: 13adbf4309bd82709c8cd54f316ed522988a1bd4' > > [ 1.974041] integrity: Revoking X.509 certificate: UEFI:MokListXRT (MOKvar table) > > [ 1.978852] blacklist: Revoked X.509 cert 'Canonical Ltd. Secure Boot Signing: 61482aa2830d0ab2ad5af10b7250da9033ddcef0' > > [ 1.985850] integrity: Loading X.509 certificate: UEFI:MokListRT (MOKvar table) > > [ 1.989651] integrity: Loaded X.509 cert 'Canonical Ltd. Master Certificate Authority: ad91990bc22ab1f517048c23b6655a268e345a63' > > Strip the [] from the dmesg. They make it harder to read. > OK > > > > Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@canonical.com> > > --- > > > > Another RFC patch that I'd like to submit upstream, based on > > v5.13-rc1. > > I think you can just send it upstream and get the comments there. It's > not confidential (already made public through kernel-team list) and > upstream (specialized) folks might know this pieces better than us. > > > > > certs/blacklist.c | 4 ++++ > > security/integrity/platform_certs/keyring_handler.c | 1 + > > 2 files changed, 5 insertions(+) > > > > diff --git a/certs/blacklist.c b/certs/blacklist.c > > index c9a435b15af40..738c496756516 100644 > > --- a/certs/blacklist.c > > +++ b/certs/blacklist.c > > @@ -172,6 +172,10 @@ int add_key_to_revocation_list(const char *data, size_t size) > > if (IS_ERR(key)) { > > pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key)); > > return PTR_ERR(key); > > + } else { > > + pr_notice("Revoked X.509 cert '%s'\n", > > + key_ref_to_ptr(key)->description); > > + key_ref_put(key); > > This seems unrelated, based on the commit description. Why do you need it? I think the original code today is leaky. I thought that after calling key_create_or_update() one should call key_ref_put(key) too..... I wonder if I should mention that in the commit message, or send it separately.
On 10/05/2021 11:04, Dimitri John Ledkov wrote: > On Mon, May 10, 2021 at 3:48 PM Krzysztof Kozlowski > <krzysztof.kozlowski@canonical.com> wrote: >> >>> diff --git a/certs/blacklist.c b/certs/blacklist.c >>> index c9a435b15af40..738c496756516 100644 >>> --- a/certs/blacklist.c >>> +++ b/certs/blacklist.c >>> @@ -172,6 +172,10 @@ int add_key_to_revocation_list(const char *data, size_t size) >>> if (IS_ERR(key)) { >>> pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key)); >>> return PTR_ERR(key); >>> + } else { >>> + pr_notice("Revoked X.509 cert '%s'\n", >>> + key_ref_to_ptr(key)->description); >>> + key_ref_put(key); >> >> This seems unrelated, based on the commit description. Why do you need it? > > I think the original code today is leaky. I thought that after calling > key_create_or_update() one should call key_ref_put(key) too..... > > I wonder if I should mention that in the commit message, or send it separately. Please send it separately with its own Fixes tag. The bugfixes should never be mixed with other patches because it stops them from backporting and makes review more difficult. Thanks! Best regards, Krzysztof
diff --git a/certs/blacklist.c b/certs/blacklist.c index c9a435b15af40..738c496756516 100644 --- a/certs/blacklist.c +++ b/certs/blacklist.c @@ -172,6 +172,10 @@ int add_key_to_revocation_list(const char *data, size_t size) if (IS_ERR(key)) { pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key)); return PTR_ERR(key); + } else { + pr_notice("Revoked X.509 cert '%s'\n", + key_ref_to_ptr(key)->description); + key_ref_put(key); } return 0; diff --git a/security/integrity/platform_certs/keyring_handler.c b/security/integrity/platform_certs/keyring_handler.c index 5604bd57c9907..9f85626702b2c 100644 --- a/security/integrity/platform_certs/keyring_handler.c +++ b/security/integrity/platform_certs/keyring_handler.c @@ -61,6 +61,7 @@ static __init void uefi_blacklist_binary(const char *source, static __init void uefi_revocation_list_x509(const char *source, const void *data, size_t len) { + pr_info("Revoking X.509 certificate: %s\n", source); add_key_to_revocation_list(data, len); }
integrity_load_cert() prints messages of the source and cert details when adding certs as trusted. Mirror those messages in uefi_revocation_list_x509() when adding certs as revoked. Sample dmesg with this change: [ 1.538741] integrity: Platform Keyring initialized [ 1.960071] integrity: Loading X.509 certificate: UEFI:db [ 1.961986] integrity: Loaded X.509 cert 'Microsoft Corporation UEFI CA 2011: 13adbf4309bd82709c8cd54f316ed522988a1bd4' [ 1.974041] integrity: Revoking X.509 certificate: UEFI:MokListXRT (MOKvar table) [ 1.978852] blacklist: Revoked X.509 cert 'Canonical Ltd. Secure Boot Signing: 61482aa2830d0ab2ad5af10b7250da9033ddcef0' [ 1.985850] integrity: Loading X.509 certificate: UEFI:MokListRT (MOKvar table) [ 1.989651] integrity: Loaded X.509 cert 'Canonical Ltd. Master Certificate Authority: ad91990bc22ab1f517048c23b6655a268e345a63' Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@canonical.com> --- Another RFC patch that I'd like to submit upstream, based on v5.13-rc1. certs/blacklist.c | 4 ++++ security/integrity/platform_certs/keyring_handler.c | 1 + 2 files changed, 5 insertions(+)