| Message ID | 20231227044156.166009-1-coxu@redhat.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | integrity: don't throw an error immediately when failed to add a cert to the .machine keyring | expand |
> On Dec 26, 2023, at 9:41 PM, Coiby Xu <coxu@redhat.com> wrote: > > Currently when the kernel fails to add a cert to the .machine keyring, > it will throw an error immediately in the function integrity_add_key. > > Since the kernel will try adding to the .platform keyring next or throw > an error (in the caller of integrity_add_key i.e. add_to_machine_keyring), > so there is no need to throw an error immediately in integrity_add_key. > > Reported-by: itrymybest80@protonmail.com > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331 > Signed-off-by: Coiby Xu <coxu@redhat.com> Reviewed-by: Eric Snowberg <eric.snowberg@oracle.com>
Hi Coiby, According to https://docs.kernel.org/process/submitting-patches.html,the summary line should be no more than 70 - 75 characters. On Wed, 2023-12-27 at 12:41 +0800, Coiby Xu wrote: > Currently when the kernel fails to add a cert to the .machine keyring, > it will throw an error immediately in the function integrity_add_key. > > Since the kernel will try adding to the .platform keyring next or throw > an error (in the caller of integrity_add_key i.e. add_to_machine_keyring), > so there is no need to throw an error immediately in integrity_add_key. > > Reported-by: itrymybest80@protonmail.com > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331 > Signed-off-by: Coiby Xu <coxu@redhat.com> Otherwise, the patch looks good.
On Wed Dec 27, 2023 at 6:41 AM EET, Coiby Xu wrote: > Currently when the kernel fails to add a cert to the .machine keyring, > it will throw an error immediately in the function integrity_add_key. > > Since the kernel will try adding to the .platform keyring next or throw > an error (in the caller of integrity_add_key i.e. add_to_machine_keyring), > so there is no need to throw an error immediately in integrity_add_key. > > Reported-by: itrymybest80@protonmail.com Missing "Firstname Lastname". > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331 > Signed-off-by: Coiby Xu <coxu@redhat.com> > --- > security/integrity/digsig.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c > index df387de29bfa..45c3e5dda355 100644 > --- a/security/integrity/digsig.c > +++ b/security/integrity/digsig.c > @@ -179,7 +179,8 @@ static int __init integrity_add_key(const unsigned int id, const void *data, > KEY_ALLOC_NOT_IN_QUOTA); > if (IS_ERR(key)) { > rc = PTR_ERR(key); > - pr_err("Problem loading X.509 certificate %d\n", rc); > + if (id != INTEGRITY_KEYRING_MACHINE) > + pr_err("Problem loading X.509 certificate %d\n", rc); > } else { > pr_notice("Loaded X.509 cert '%s'\n", > key_ref_to_ptr(key)->description); BR, Jarkko
On Wed, Jan 03, 2024 at 04:09:29PM +0200, Jarkko Sakkinen wrote: >On Wed Dec 27, 2023 at 6:41 AM EET, Coiby Xu wrote: >> Currently when the kernel fails to add a cert to the .machine keyring, >> it will throw an error immediately in the function integrity_add_key. >> >> Since the kernel will try adding to the .platform keyring next or throw >> an error (in the caller of integrity_add_key i.e. add_to_machine_keyring), >> so there is no need to throw an error immediately in integrity_add_key. >> >> Reported-by: itrymybest80@protonmail.com > >Missing "Firstname Lastname". Thanks for raising this concern! I've asked the reporter if he/she can share his/her name.
On Tue, Jan 02, 2024 at 12:54:02PM -0500, Mimi Zohar wrote: >Hi Coiby, Hi Mimi, > >According to https://docs.kernel.org/process/submitting-patches.html,the summary line should be no more than 70 - 75 characters. Thanks for pointing me to this limit! How about integrity: eliminate harmless error "Problem loading X.509 certificate -126"? > >On Wed, 2023-12-27 at 12:41 +0800, Coiby Xu wrote: >> Currently when the kernel fails to add a cert to the .machine keyring, >> it will throw an error immediately in the function integrity_add_key. >> >> Since the kernel will try adding to the .platform keyring next or throw >> an error (in the caller of integrity_add_key i.e. add_to_machine_keyring), >> so there is no need to throw an error immediately in integrity_add_key. >> >> Reported-by: itrymybest80@protonmail.com >> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331 >> Signed-off-by: Coiby Xu <coxu@redhat.com> > >Otherwise, the patch looks good. Thanks for reviewing the patch! > >-- >thanks, > >Mim > > >
On Tue, Jan 02, 2024 at 05:33:53PM +0000, Eric Snowberg wrote: > > >> On Dec 26, 2023, at 9:41 PM, Coiby Xu <coxu@redhat.com> wrote: >> >> Currently when the kernel fails to add a cert to the .machine keyring, >> it will throw an error immediately in the function integrity_add_key. >> >> Since the kernel will try adding to the .platform keyring next or throw >> an error (in the caller of integrity_add_key i.e. add_to_machine_keyring), >> so there is no need to throw an error immediately in integrity_add_key. >> >> Reported-by: itrymybest80@protonmail.com >> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331 >> Signed-off-by: Coiby Xu <coxu@redhat.com> > >Reviewed-by: Eric Snowberg <eric.snowberg@oracle.com> Thank you for reviewing the patch!
On Fri, 2024-01-05 at 21:27 +0800, Coiby Xu wrote: > On Tue, Jan 02, 2024 at 12:54:02PM -0500, Mimi Zohar wrote: > >Hi Coiby, > > Hi Mimi, > > > > >According to https://docs.kernel.org/process/submitting-patches.html,the > summary line should be no more than 70 - 75 characters. > > Thanks for pointing me to this limit! How about > integrity: eliminate harmless error "Problem loading X.509 certificate -126" Still >75. How about the following? integrity: eliminate unnecessary "Problem loading X.509 certificate" msg Mimi > > > > >On Wed, 2023-12-27 at 12:41 +0800, Coiby Xu wrote: > >> Currently when the kernel fails to add a cert to the .machine keyring, > >> it will throw an error immediately in the function integrity_add_key. > >> > >> Since the kernel will try adding to the .platform keyring next or throw > >> an error (in the caller of integrity_add_key i.e. add_to_machine_keyring), > >> so there is no need to throw an error immediately in integrity_add_key. > >> > >> Reported-by: itrymybest80@protonmail.com > >> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331 > >> Signed-off-by: Coiby Xu <coxu@redhat.com> > > > >Otherwise, the patch looks good. > > Thanks for reviewing the patch!
On Fri Jan 5, 2024 at 3:20 PM EET, Coiby Xu wrote: > On Wed, Jan 03, 2024 at 04:09:29PM +0200, Jarkko Sakkinen wrote: > >On Wed Dec 27, 2023 at 6:41 AM EET, Coiby Xu wrote: > >> Currently when the kernel fails to add a cert to the .machine keyring, > >> it will throw an error immediately in the function integrity_add_key. > >> > >> Since the kernel will try adding to the .platform keyring next or throw > >> an error (in the caller of integrity_add_key i.e. add_to_machine_keyring), > >> so there is no need to throw an error immediately in integrity_add_key. > >> > >> Reported-by: itrymybest80@protonmail.com > > > >Missing "Firstname Lastname". > > Thanks for raising this concern! I've asked the reporter if he/she can > share his/her name. Also, it is lacking fixes tag. Fixes tag is mandatory, name part would be super nice to have :-) Since this categories as a bug fix, getting them in is 1st priority and that thus does not absolutely block applying the change. Thanks for going trouble trying to query it, however. BR, Jarkko
On Fri, Jan 05, 2024 at 06:02:38PM +0200, Jarkko Sakkinen wrote: >On Fri Jan 5, 2024 at 3:20 PM EET, Coiby Xu wrote: >> On Wed, Jan 03, 2024 at 04:09:29PM +0200, Jarkko Sakkinen wrote: >> >On Wed Dec 27, 2023 at 6:41 AM EET, Coiby Xu wrote: >> >> Currently when the kernel fails to add a cert to the .machine keyring, >> >> it will throw an error immediately in the function integrity_add_key. >> >> >> >> Since the kernel will try adding to the .platform keyring next or throw >> >> an error (in the caller of integrity_add_key i.e. add_to_machine_keyring), >> >> so there is no need to throw an error immediately in integrity_add_key. >> >> >> >> Reported-by: itrymybest80@protonmail.com >> > >> >Missing "Firstname Lastname". >> >> Thanks for raising this concern! I've asked the reporter if he/she can >> share his/her name. > >Also, it is lacking fixes tag. Thanks for catching this issue! I've included the Fixes tag in v2. > >Fixes tag is mandatory, name part would be super nice to have :-) Since >this categories as a bug fix, getting them in is 1st priority and that >thus does not absolutely block applying the change. Thanks for going >trouble trying to query it, however. Thanks for the explanation! As I still get no reply from the reporter, so I guess we have to accept the name part for now. > >BR, Jarkko >
On Fri, Jan 05, 2024 at 09:59:14AM -0500, Mimi Zohar wrote: >On Fri, 2024-01-05 at 21:27 +0800, Coiby Xu wrote: >> On Tue, Jan 02, 2024 at 12:54:02PM -0500, Mimi Zohar wrote: >> >Hi Coiby, >> >> Hi Mimi, >> >> > >> >According to https://docs.kernel.org/process/submitting-patches.html,the >> summary line should be no more than 70 - 75 characters. >> >> Thanks for pointing me to this limit! How about >> integrity: eliminate harmless error "Problem loading X.509 certificate -126" > >Still >75. How about the following? > >integrity: eliminate unnecessary "Problem loading X.509 certificate" msg Thanks, v2 now uses the above subject. I thought the limit applies to the "summary phrase" instead of the whole "summary" and a second look proved me wrong.
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c index df387de29bfa..45c3e5dda355 100644 --- a/security/integrity/digsig.c +++ b/security/integrity/digsig.c @@ -179,7 +179,8 @@ static int __init integrity_add_key(const unsigned int id, const void *data, KEY_ALLOC_NOT_IN_QUOTA); if (IS_ERR(key)) { rc = PTR_ERR(key); - pr_err("Problem loading X.509 certificate %d\n", rc); + if (id != INTEGRITY_KEYRING_MACHINE) + pr_err("Problem loading X.509 certificate %d\n", rc); } else { pr_notice("Loaded X.509 cert '%s'\n", key_ref_to_ptr(key)->description);
Currently when the kernel fails to add a cert to the .machine keyring, it will throw an error immediately in the function integrity_add_key. Since the kernel will try adding to the .platform keyring next or throw an error (in the caller of integrity_add_key i.e. add_to_machine_keyring), so there is no need to throw an error immediately in integrity_add_key. Reported-by: itrymybest80@protonmail.com Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331 Signed-off-by: Coiby Xu <coxu@redhat.com> --- security/integrity/digsig.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)