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;
Date:   Mon, 28 Nov 2022 02:59:20 +0100
From:   Thomas =?utf-8?Q?Wei=C3=9Fschuh?= <linux@weissschuh.net>
To:     Jarkko Sakkinen <jarkko@kernel.org>
Cc:     =?utf-8?Q?Micka=C3=ABl_Sala=C3=BCn?= <mic@digikod.net>,
        David Howells <dhowells@redhat.com>,
        David Woodhouse <dwmw2@infradead.org>,
        Eric Snowberg <eric.snowberg@oracle.com>,
        keyrings@vger.kernel.org, linux-kernel@vger.kernel.org,
        Mark Pearson <markpearson@lenovo.com>,
        linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org
Subject: Re: [PATCH v3 1/3] certs: log hash value on blacklist error
Message-ID: <8b9e9bf8-ae44-485a-9b30-85a71a236f06@t-8ch.de>
References: <20221118040343.2958-1-linux@weissschuh.net>
 <20221118040343.2958-2-linux@weissschuh.net>
 <Y4QK2cmptp4vpRj/@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <Y4QK2cmptp4vpRj/@kernel.org>
Jabber-ID: thomas@t-8ch.de
Precedence: bulk

On 2022-11-28 03:11+0200, Jarkko Sakkinen wrote:
> "Make blacklisted hash available in klog"
> 
> On Fri, Nov 18, 2022 at 05:03:41AM +0100, Thomas Weißschuh wrote:
> > Without this information these logs are not actionable.
> 
> Without blacklisted hash?
> 
> > For example on duplicate blacklisted hashes reported by the system
> > firmware users should be able to report the erroneous hashes to their
> > system vendors.
> > 
> > While we are at it use the dedicated format string for ERR_PTR.
> 
> Lacks the beef so saying "while we are at it" makes no sense.

What about this:

  [PATCH] certs: make blacklisted hash available in klog

  One common situation triggering this log statement are duplicate hashes
  reported by the system firmware.

  These duplicates should be removed from the firmware.

  Without logging the blacklisted hash triggering the issue however the users
  can not report it properly to the firmware vendors and the firmware vendors
  can not easily see which specific hash is duplicated.

  While changing the log message also use the dedicated ERR_PTR format
  placeholder for the returned error value.

> > Fixes: 6364d106e041 ("certs: Allow root user to append signed hashes to the blacklist keyring")
> 
> Why does this count as a bug?

These error logs are confusing to users, prompting them to waste time
investigating them and even mess with their firmware settings.
(As indicated in the threads linked from the cover letter)

The most correct fix would be patches 2 and 3 from this series.

I was not sure if patch 2 would be acceptable for stable as it introduces new
infrastructure code.
So patch 1 enables users to report the issue to their firmware vendors and get
the spurious logs resolved that way.

If these assumptions are incorrect I can fold patch 1 into patch 3.

But are patch 2 and 3 material for stable?

> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >  certs/blacklist.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/certs/blacklist.c b/certs/blacklist.c
> > index 41f10601cc72..6e260c4b6a19 100644
> > --- a/certs/blacklist.c
> > +++ b/certs/blacklist.c
> > @@ -192,7 +192,7 @@ static int mark_raw_hash_blacklisted(const char *hash)
> >  				   KEY_ALLOC_NOT_IN_QUOTA |
> >  				   KEY_ALLOC_BUILT_IN);
> >  	if (IS_ERR(key)) {
> > -		pr_err("Problem blacklisting hash (%ld)\n", PTR_ERR(key));
> > +		pr_err("Problem blacklisting hash %s: %pe\n", hash, key);
> >  		return PTR_ERR(key);
> >  	}
> >  	return 0;
> > -- 
> > 2.38.1
> > 
> 
> BR, Jarkko