Message ID | 20221104014704.3469-1-linux@weissschuh.net |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp108537wru; Thu, 3 Nov 2022 18:52:29 -0700 (PDT) X-Google-Smtp-Source: AMsMyM5BHM7A58B0s1cxoqTOLwU7ZOqV+xfSEvSR5tRzBpYt/H2fLxFlCT281CJ6FRAEqmS7pWYS X-Received: by 2002:a17:906:5a6a:b0:7a6:bad5:8295 with SMTP id my42-20020a1709065a6a00b007a6bad58295mr32354512ejc.647.1667526748870; Thu, 03 Nov 2022 18:52:28 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1667526748; cv=none; d=google.com; s=arc-20160816; b=pAQTgciGEwntjaOb5zLus5lIr7IQzcrtBbyku390TtxnN0yoGvTKIGfLKBpwrtKQnY D3h+fevlYJnquelT9UKYy76Up0U+ApDxLj6sGCh6Utt/ey4xEB58IFylniU8CoOtlUwo GkvinsqA/rKQEuPsic/HtgD4qQr0v/t65+h4zy1QoT2piG5B9WFwE4vo06lWcxXqDVzc AVe2kwZofvms0fc0wVvIsLWLSHi9n6XV0eyvSanuOUrP9GUDJvIVkKomHU0JhPtv1vdu QtBfVvwDdkB978RJzQGWMXyHBbzcHAAPZkJ77K4Ezj0GxKIOTZ/AnnV2xFYDXax0scxb EnyQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:dkim-signature:from; bh=xZPcQBiWXFjnWO8MPgkbTDO/BmT+oq4ipkIu1SUdyiI=; b=vWxYquLC54wGgjcrlKlPTv7+rKWC05lQw64Dmm8wxeGMz/ec5QkJw1HCSwC75urTmP p2+HmTUQviNrB6CFqnPZLWbzzD2p0Wn+qjhjFLaj8Q6hcBN88bH+FLDj8xLBlJDy/7Yq N+qN2hN8CClJPkxIdAIZuYzg+pFX4sCoYyI1/An0GcbErNNrsssQd32ELBOgfu2DBIJU NN0hw2HBB6udFRyh9dzfaANRQnZV+yng8I5LfNQ3OMchcd2zePgCBoCtLhE+YHcpF9d8 Of2en2dCTkxx0Wc84E1/rrNs15W2dx4JTzcv72wYW3ReD/ABxbwpzYiLbRyGIXqJGqrs zEfw== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@weissschuh.net header.s=mail header.b=Rti15vTq; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id dp15-20020a170906c14f00b007add62d9f38si3147612ejc.330.2022.11.03.18.52.05; Thu, 03 Nov 2022 18:52:28 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=fail header.i=@weissschuh.net header.s=mail header.b=Rti15vTq; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229637AbiKDBr2 (ORCPT <rfc822;jimliu8233@gmail.com> + 99 others); Thu, 3 Nov 2022 21:47:28 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42692 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229481AbiKDBr0 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 3 Nov 2022 21:47:26 -0400 Received: from todd.t-8ch.de (todd.t-8ch.de [159.69.126.157]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 72384233AB; Thu, 3 Nov 2022 18:47:25 -0700 (PDT) From: =?utf-8?q?Thomas_Wei=C3=9Fschuh?= <linux@weissschuh.net> DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=weissschuh.net; s=mail; t=1667526441; bh=zA6BOM4Lon4YmACr2oprtmCrkYDcna5kP8A9nDzjIyQ=; h=From:To:Cc:Subject:Date:From; b=Rti15vTqHTmnoxSOumHx9nR+wOhVCzHGsJIGz/GjeEvqALRluQ4JmqaFTXSR9/ZsO 3Wgf3vdUZBQe9fpW+3MtGDX6/yarLXYRxp4YHXHnQ05fMtpqMwKoPt8eSdrMdvwUiQ Oi/xQpDov5sB+8j+Hp15a3fPfUhB5X52ZKdxs920= To: David Howells <dhowells@redhat.com>, David Woodhouse <dwmw2@infradead.org> Cc: =?utf-8?q?Thomas_Wei=C3=9Fschuh?= <linux@weissschuh.net>, keyrings@vger.kernel.org, linux-kernel@vger.kernel.org, =?utf-8?q?Micka?= =?utf-8?q?=C3=ABl_Sala=C3=BCn?= <mic@linux.microsoft.com> Subject: [PATCH] certs: Prevent spurious errors on repeated blacklisting Date: Fri, 4 Nov 2022 02:47:04 +0100 Message-Id: <20221104014704.3469-1-linux@weissschuh.net> X-Mailer: git-send-email 2.38.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 X-Developer-Signature: v=1; a=ed25519-sha256; t=1667526395; l=1345; s=20211113; h=from:subject; bh=zA6BOM4Lon4YmACr2oprtmCrkYDcna5kP8A9nDzjIyQ=; b=klhYPd0ukYGk28HhUv7Q+VCjWvIYNkiMK7eI+uQbQZQrX6nT9RAW7Usn0s9WXbbblQ8jFYeFg58y 3tOqsWOzDZ96hlDb8JcaHzxd+HDEHfWnSNhGJxtB3pcyTuA8wN5K X-Developer-Key: i=linux@weissschuh.net; a=ed25519; pk=9LP6KM4vD/8CwHW7nouRBhWLyQLcK1MkP6aTZbzUlj4= Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1748528528226841771?= X-GMAIL-MSGID: =?utf-8?q?1748528528226841771?= |
Series |
certs: Prevent spurious errors on repeated blacklisting
|
|
Commit Message
Thomas Weißschuh
Nov. 4, 2022, 1:47 a.m. UTC
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.
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(-)
base-commit: ee6050c8af96bba2f81e8b0793a1fc2f998fcd20
Comments
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. > > 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. > pr_err("Problem blacklisting hash (%ld)\n", PTR_ERR(key)); > return PTR_ERR(key); > } > > base-commit: ee6050c8af96bba2f81e8b0793a1fc2f998fcd20
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 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) 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. > > pr_err("Problem blacklisting hash (%ld)\n", PTR_ERR(key)); > > return PTR_ERR(key); > > } > > > > base-commit: ee6050c8af96bba2f81e8b0793a1fc2f998fcd20
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
On 2022-11-07 17:20+0100, Mickaël Salaün wrote: > 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? Indeed, sorry. > > 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. Will do. My plan is to create a new function key_create() that does takes the core logic of key_create_or_update() and fails with EEXIST if needed. > > 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 I'll send a patch for that, too. As for this patch's Fixes-tag, it could refer to either the commit that introduced the logging in the first place or the one that actively started to trigger it: * 734114f8782f ("KEYS: Add a system blacklist keyring") * 6364d106e041 ("certs: Allow root user to append signed hashes to the blacklist keyring") Personally I'd tend to use the latter. > > > > pr_err("Problem blacklisting hash (%ld)\n", PTR_ERR(key)); > > > > return PTR_ERR(key); > > > > } > > > > > > > > base-commit: ee6050c8af96bba2f81e8b0793a1fc2f998fcd20
On 07/11/2022 17:35, Thomas Weißschuh wrote: > On 2022-11-07 17:20+0100, Mickaël Salaün wrote: >> 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? > > Indeed, sorry. > >>> 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. > > Will do. > > My plan is to create a new function key_create() that does takes the core logic > of key_create_or_update() and fails with EEXIST if needed. > >>> 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 > > I'll send a patch for that, too. Good! Jarkko, David, any though? > > As for this patch's Fixes-tag, it could refer to either the commit that > introduced the logging in the first place or the one that actively started to > trigger it: > * 734114f8782f ("KEYS: Add a system blacklist keyring") > * 6364d106e041 ("certs: Allow root user to append signed hashes to the blacklist keyring") > > Personally I'd tend to use the latter. Even if commit 6364d106e041 is not directly the cause of the issue, it makes it visible, so I agree that you should keep the current Fixes tag. > >>>>> pr_err("Problem blacklisting hash (%ld)\n", PTR_ERR(key)); >>>>> return PTR_ERR(key); >>>>> } >>>>> >>>>> base-commit: ee6050c8af96bba2f81e8b0793a1fc2f998fcd20
> On Nov 7, 2022, at 6:12 AM, Mickaël Salaün <mic@digikod.net> 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. I have seen error reports like this in the past. Some of the older UEFI Revocation List files up on the UEFI.org site [1] contain duplicates. If a firmware vendor uses one of these older Microsoft signed files, they are going to see these error messages. 1. https://uefi.org/revocationlistfile/archive
On Mon, Nov 07, 2022 at 08:40:09PM +0100, Mickaël Salaün wrote: > > On 07/11/2022 17:35, Thomas Weißschuh wrote: > > On 2022-11-07 17:20+0100, Mickaël Salaün wrote: > > > 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? > > > > Indeed, sorry. > > > > > > 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. > > > > Will do. > > > > My plan is to create a new function key_create() that does takes the core logic > > of key_create_or_update() and fails with EEXIST if needed. > > > > > > 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 > > > > I'll send a patch for that, too. > > Good! > > Jarkko, David, any though? I'm happy to review a patch once it is available. BR, Jarkko
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) { pr_err("Problem blacklisting hash (%ld)\n", PTR_ERR(key)); return PTR_ERR(key); }