linux-kernel - Re: [PATCH v1 2/3] firmware: coreboot: loader for Linux-owned SMI handler

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aFBdfckccRv7Pbc6@google.com>
Date: Mon, 16 Jun 2025 11:07:57 -0700
From: Brian Norris <briannorris@...omium.org>
To: Michal Gorlas <michal.gorlas@...ements.com>
Cc: Tzung-Bi Shih <tzungbi@...nel.org>,
	Julius Werner <jwerner@...omium.org>, marcello.bauer@...ements.com,
	chrome-platform@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/3] firmware: coreboot: loader for Linux-owned SMI
 handler

On Sat, Jun 14, 2025 at 02:59:33PM +0200, Michal Gorlas wrote:
> On Thu, Jun 12, 2025 at 03:38:21PM -0700, Brian Norris wrote:
> > > +	mdelay(100);
> > 
> > Why the delay? At least use a comment to tell us. And if it's really
> > needed, use msleep(), not mdelay(). scripts/checkpatch.pl should have
> > warned you. And, please use scripts/checkpatch.pl if you aren't already
> > ;)
> > 
> 
> Long story short, SMIs on real hardware like to take longer from time to
> time, and the delay was a "safeguard". It is probably not the proper way
> to handle it, but locking here was not helpful at all, lock was released
> regardless of CPU being still in SMM context (I assume due to SMIs being
> invisible to whatever runs in ring-0). Have to admit though, that 100ms
> is a consequence of trial and error. I would actually use some on advice
> how to handle this properly.

Sorry, I don't have any advice here at the moment.

> scripts/checkpatch.pl was not complaining
> about it. It only gave me:
> 
> WARNING: quoted string split across lines
> #57: FILE: drivers/firmware/google/mm_loader.c:57:
> +		     ".return_not_changed:"
> +		     "movq	%%rcx, %[status]\n\t"
> 
> total: 0 errors, 1 warnings, 0 checks, 186 lines checked

I must have either misread or misremembered checkpatch's behavior.
Possibly both. It has various other delay-realted warnings that point
you at the kerneldoc comments for mdelay() and msleep() though, and the
mdelay() comments say:

 * Please double check, whether mdelay() is the right way to go or whether a
 * refactoring of the code is the better variant to be able to use msleep()
 * instead.

Brian