Messages in this thread | | | Date | Mon, 7 Nov 2022 17:20:53 +0100 | Subject | Re: [PATCH] certs: Prevent spurious errors on repeated blacklisting | From | Mickaël Salaün <> |
| |
On 07/11/2022 16:55, Thomas Weißschuh wrote: > On 2022-11-07 14:12+0100, Mickaël Salaün wrote: >> This is a follow-up of >> https://lore.kernel.org/r/c8c65713-5cda-43ad-8018-20f2e32e4432@t-8ch.de >> >> Added Jarkko, Mark Pearson, Eric Snowberg and more ML in Cc. >> >> >> On 04/11/2022 02:47, Thomas Weißschuh wrote: >>> When the blacklist keyring was changed to allow updates from the root >>> user it gained an ->update() function that disallows all updates. >>> When the a hash is blacklisted multiple times from the builtin or >>> firmware-provided blacklist this spams prominent logs during boot: >>> >>> [ 0.890814] blacklist: Problem blacklisting hash (-13) >>> >>> As all these repeated calls to mark_raw_hash_blacklisted() would create >>> the same keyring entry again anyways these errors can be safely ignored. >> >> These errors can indeed be safely ignored, however they highlight issues >> with some firmware vendors not checking nor optimizing their blocked hashes. >> This raises security concerns, and it should be fixed by firmware vendors. > > Thanks, I was not aware that these are worth fixing. > >>> Fixes: 6364d106e041 ("certs: Allow root user to append signed hashes to the blacklist keyring") >>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> >>> --- >>> certs/blacklist.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/certs/blacklist.c b/certs/blacklist.c >>> index 41f10601cc72..5f7f2882ced7 100644 >>> --- a/certs/blacklist.c >>> +++ b/certs/blacklist.c >>> @@ -191,7 +191,9 @@ static int mark_raw_hash_blacklisted(const char *hash) >>> BLACKLIST_KEY_PERM, >>> KEY_ALLOC_NOT_IN_QUOTA | >>> KEY_ALLOC_BUILT_IN); >>> - if (IS_ERR(key)) { >>> + >>> + /* Blacklisting the same hash twice fails but would be idempotent */ >>> + if (IS_ERR(key) && PTR_ERR(key) != -EACCES) { >> >> We should not hide EACCES errors. This logs issues, which is correct for >> duplicate hashes, and can help firmware vendors to fix their database. I'd >> really like to see a different log message instead: change the duplicate >> entry error code from EACCES to EEXIST, and call pr_warn for this specific >> case. > > Returning EACCES would require some deeper changes to how the keyring is set up
I guess you meant EEXIST?
> or even changes to the keyring core itself to introduce a key_create() (without > update) function. > > Is this something you would take a look at, or should I try to do it? > (I have no previous knowledge about the keyring subsystem)
Please take a look. I think it should not be too complex.
> > In any case it probably would also be good to log the problematic hashes > themselves, so users can properly report the issue to their firmware vendors.
Agree
> >>> pr_err("Problem blacklisting hash (%ld)\n", PTR_ERR(key)); >>> return PTR_ERR(key); >>> } >>> >>> base-commit: ee6050c8af96bba2f81e8b0793a1fc2f998fcd20
| |