[PATCH] integrity: don't throw an error immediately when failed to add a cert to the .machine keyring

[PATCH] integrity: don't throw an error immediately when failed to add a cert to the .machine keyring

[Coiby Xu]

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(-)

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

Re: [PATCH] integrity: don't throw an error immediately when failed to add a cert to the .machine keyring

[Eric Snowberg]

> 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>

Re: Re: [PATCH] integrity: don't throw an error immediately when failed to add a cert to the .machine keyring

[Coiby Xu]

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!

-- 
Best regards,
Coiby

Re: [PATCH] integrity: don't throw an error immediately when failed to add a cert to the .machine keyring

[Mimi Zohar]

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.

-- 
thanks,

Mim

Re: Re: [PATCH] integrity: don't throw an error immediately when failed to add a cert to the .machine keyring

[Coiby Xu]

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
>
>
>

-- 
Best regards,
Coiby

Re: Re: [PATCH] integrity: don't throw an error immediately when failed to add a cert to the .machine keyring

[Mimi Zohar]

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!

Re: Re: Re: [PATCH] integrity: don't throw an error immediately when failed to add a cert to the .machine keyring

[Coiby Xu]

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

Re: [PATCH] integrity: don't throw an error immediately when failed to add a cert to the .machine keyring

[Jarkko Sakkinen]

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

Re: Re: [PATCH] integrity: don't throw an error immediately when failed to add a cert to the .machine keyring

[Coiby Xu]

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.

-- 
Best regards,
Coiby

Re: [PATCH] integrity: don't throw an error immediately when failed to add a cert to the .machine keyring

[Jarkko Sakkinen]

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

Re: Re: [PATCH] integrity: don't throw an error immediately when failed to add a cert to the .machine keyring

[Coiby Xu]

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
>

-- 
Best regards,
Coiby

[PATCH v2] integrity: eliminate unnecessary "Problem loading X.509 certificate" msg

[Coiby Xu]

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
Fixes: d19967764ba8 ("integrity: Introduce a Linux keyring called machine")
Reviewed-by: Eric Snowberg <eric.snowberg@oracle.com>
Signed-off-by: Coiby Xu <coxu@redhat.com>
---
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

Re: [PATCH v2] integrity: eliminate unnecessary "Problem loading X.509 certificate" msg

[Coiby Xu]

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: itrymybest80@protonmail.com
>Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331
>Fixes: d19967764ba8 ("integrity: Introduce a Linux keyring called machine")
>Reviewed-by: Eric Snowberg <eric.snowberg@oracle.com>
>Signed-off-by: Coiby Xu <coxu@redhat.com>
>---
>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

Re: [PATCH v2] integrity: eliminate unnecessary "Problem loading X.509 certificate" msg

[Mimi Zohar]

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

Re: [PATCH v2] integrity: eliminate unnecessary "Problem loading X.509 certificate" msg

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 886 bytes --]

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: itrymybest80@protonmail.com
> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331
> Fixes: d19967764ba8 ("integrity: Introduce a Linux keyring called machine")
> Reviewed-by: Eric Snowberg <eric.snowberg@oracle.com>
> Signed-off-by: Coiby Xu <coxu@redhat.com>

Any chance this patch can be merged? This is breaking (at least) Fedora
at the moment.

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] integrity: eliminate unnecessary "Problem loading X.509 certificate" msg

[Mimi Zohar]

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: itrymybest80@protonmail.com
> > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331
> > Fixes: d19967764ba8 ("integrity: Introduce a Linux keyring called machine")
> > Reviewed-by: Eric Snowberg <eric.snowberg@oracle.com>
> > Signed-off-by: Coiby Xu <coxu@redhat.com>
> 
> Any chance this patch can be merged? This is breaking (at least) Fedora
> at the moment.

https://git.kernel.org/torvalds/c/29cd507cbec282e13dcf8f38072a100af96b2bb7

Mimi

Re: [PATCH v2] integrity: eliminate unnecessary "Problem loading X.509 certificate" msg

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

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: itrymybest80@protonmail.com
> > > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331
> > > Fixes: d19967764ba8 ("integrity: Introduce a Linux keyring called machine")
> > > Reviewed-by: Eric Snowberg <eric.snowberg@oracle.com>
> > > Signed-off-by: Coiby Xu <coxu@redhat.com>
> > 
> > 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] integrity: eliminate unnecessary "Problem loading X.509 certificate" msg

[Coiby Xu]

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: itrymybest80@protonmail.com
>> > > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2239331
>> > > Fixes: d19967764ba8 ("integrity: Introduce a Linux keyring called machine")
>> > > Reviewed-by: Eric Snowberg <eric.snowberg@oracle.com>
>> > > Signed-off-by: Coiby Xu <coxu@redhat.com>
>> >
>> > 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