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: [email protected]
Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331
Signed-off-by: Coiby Xu <[email protected]>
---
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);
--
2.43.0
> On Dec 26, 2023, at 9:41 PM, Coiby Xu <[email protected]> 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: [email protected]
> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331
> Signed-off-by: Coiby Xu <[email protected]>
Reviewed-by: Eric Snowberg <[email protected]>
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: [email protected]
> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331
> Signed-off-by: Coiby Xu <[email protected]>
Otherwise, the patch looks good.
--
thanks,
Mim
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: [email protected]
Missing "Firstname Lastname".
> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331
> Signed-off-by: Coiby Xu <[email protected]>
> ---
> 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: [email protected]
>
>Missing "Firstname Lastname".
Thanks for raising this concern! I've asked the reporter if he/she can
share his/her name.
--
Best regards,
Coiby
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: [email protected]
>> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331
>> Signed-off-by: Coiby Xu <[email protected]>
>
>Otherwise, the patch looks good.
Thanks for reviewing the patch!
>
>--
>thanks,
>
>Mim
>
>
>
--
Best regards,
Coiby
On Tue, Jan 02, 2024 at 05:33:53PM +0000, Eric Snowberg wrote:
>
>
>> On Dec 26, 2023, at 9:41 PM, Coiby Xu <[email protected]> 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: [email protected]
>> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331
>> Signed-off-by: Coiby Xu <[email protected]>
>
>Reviewed-by: Eric Snowberg <[email protected]>
Thank you for reviewing the patch!
--
Best regards,
Coiby
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: [email protected]
> >> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331
> >> Signed-off-by: Coiby Xu <[email protected]>
> >
> >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: [email protected]
> >
> >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
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: [email protected]
Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331
Fixes: d19967764ba8 ("integrity: Introduce a Linux keyring called machine")
Reviewed-by: Eric Snowberg <[email protected]>
Signed-off-by: Coiby Xu <[email protected]>
---
v2
- improve patch subject [Mimi]
- add Fixes tag [Jarkko]
- add Reviewed-by tag from Eric
---
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);
--
2.43.0
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: [email protected]
>> >
>> >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
>
--
Best regards,
Coiby
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.
--
Best regards,
Coiby
Hi Mimi,
Could you take a look at this version of patch? If it escaped your
attention because it got buried in the same thread, sorry for that. And
I won't send new version as a reply to previous version in the future.
On Tue, Jan 09, 2024 at 08:24:28AM +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: [email protected]
>Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331
>Fixes: d19967764ba8 ("integrity: Introduce a Linux keyring called machine")
>Reviewed-by: Eric Snowberg <[email protected]>
>Signed-off-by: Coiby Xu <[email protected]>
>---
>v2
> - improve patch subject [Mimi]
> - add Fixes tag [Jarkko]
> - add Reviewed-by tag from Eric
>---
> 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);
>--
>2.43.0
>
--
Best regards,
Coiby
On Fri, 2024-02-16 at 19:10 +0800, Coiby Xu wrote:
> Hi Mimi,
>
> Could you take a look at this version of patch? If it escaped your
> attention because it got buried in the same thread, sorry for that. And
> I won't send new version as a reply to previous version in the future.
Thanks for the reminder.
Mimi
Hi Dmitry, Eric, James, Mimi, Paul, Serge,
On Tue, Jan 09, 2024 at 08:24:28AM +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: [email protected]
> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331
> Fixes: d19967764ba8 ("integrity: Introduce a Linux keyring called machine")
> Reviewed-by: Eric Snowberg <[email protected]>
> Signed-off-by: Coiby Xu <[email protected]>
Any chance this patch can be merged? This is breaking (at least) Fedora
at the moment.
Thanks!
Maxime
On Wed, 2024-03-06 at 11:57 +0100, Maxime Ripard wrote:
> Hi Dmitry, Eric, James, Mimi, Paul, Serge,
>
> On Tue, Jan 09, 2024 at 08:24:28AM +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: [email protected]
> > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331
> > Fixes: d19967764ba8 ("integrity: Introduce a Linux keyring called machine")
> > Reviewed-by: Eric Snowberg <[email protected]>
> > Signed-off-by: Coiby Xu <[email protected]>
>
> Any chance this patch can be merged? This is breaking (at least) Fedora
> at the moment.
https://git.kernel.org/torvalds/c/29cd507cbec282e13dcf8f38072a100af96b2bb7
Mimi
On Wed, Mar 06, 2024 at 06:55:00AM -0500, Mimi Zohar wrote:
> On Wed, 2024-03-06 at 11:57 +0100, Maxime Ripard wrote:
> > Hi Dmitry, Eric, James, Mimi, Paul, Serge,
> >
> > On Tue, Jan 09, 2024 at 08:24:28AM +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: [email protected]
> > > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331
> > > Fixes: d19967764ba8 ("integrity: Introduce a Linux keyring called machine")
> > > Reviewed-by: Eric Snowberg <[email protected]>
> > > Signed-off-by: Coiby Xu <[email protected]>
> >
> > Any chance this patch can be merged? This is breaking (at least) Fedora
> > at the moment.
>
> https://git.kernel.org/torvalds/c/29cd507cbec282e13dcf8f38072a100af96b2bb7
Oh, awesome, we missed it.
Thanks!
Maxime
On Wed, Mar 06, 2024 at 01:40:01PM +0100, Maxime Ripard wrote:
>On Wed, Mar 06, 2024 at 06:55:00AM -0500, Mimi Zohar wrote:
>> On Wed, 2024-03-06 at 11:57 +0100, Maxime Ripard wrote:
>> > Hi Dmitry, Eric, James, Mimi, Paul, Serge,
>> >
>> > On Tue, Jan 09, 2024 at 08:24:28AM +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: [email protected]
>> > > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331
>> > > Fixes: d19967764ba8 ("integrity: Introduce a Linux keyring called machine")
>> > > Reviewed-by: Eric Snowberg <[email protected]>
>> > > Signed-off-by: Coiby Xu <[email protected]>
>> >
>> > Any chance this patch can be merged? This is breaking (at least) Fedora
>> > at the moment.
>>
>> https://git.kernel.org/torvalds/c/29cd507cbec282e13dcf8f38072a100af96b2bb7
>
>Oh, awesome, we missed it.
Oh, I missed the emails about Mimi's PR sent to Linus as well. Btw, I'm
curious to ask why you used the word "breaking" because I thought these KERN_ERR
errors shouldn't cause any real problem.
--
Best regards,
Coiby