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;
Message-ID: <3b997266-067c-975c-911a-da146fe9033a@digikod.net>
Date:   Mon, 7 Nov 2022 14:12:13 +0100
MIME-Version: 1.0
User-Agent: 
Subject: Re: [PATCH] certs: Prevent spurious errors on repeated blacklisting
Content-Language: en-US
To:     =?UTF-8?Q?Thomas_Wei=c3=9fschuh?= <linux@weissschuh.net>,
        David Howells <dhowells@redhat.com>,
        David Woodhouse <dwmw2@infradead.org>,
        Jarkko Sakkinen <jarkko@kernel.org>
Cc:     keyrings@vger.kernel.org, linux-kernel@vger.kernel.org,
        Mark Pearson <markpearson@lenovo.com>,
        Eric Snowberg <eric.snowberg@oracle.com>,
        linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org
References: <20221104014704.3469-1-linux@weissschuh.net>
From:   =?UTF-8?Q?Micka=c3=abl_Sala=c3=bcn?= <mic@digikod.net>
In-Reply-To: <20221104014704.3469-1-linux@weissschuh.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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