Date:   Wed, 1 Feb 2023 07:42:34 -0800
From:   Ashok Raj <ashok.raj@intel.com>
To:     Borislav Petkov <bp@alien8.de>
CC:     Thomas Gleixner <tglx@linutronix.de>,
        LKML <linux-kernel@vger.kernel.org>, x86 <x86@kernel.org>,
        Ingo Molnar <mingo@kernel.org>,
        Tony Luck <tony.luck@intel.com>,
        Dave Hansen <dave.hansen@intel.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Reinette Chatre <reinette.chatre@intel.com>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        Stefan Talpalaru <stefantalpalaru@yahoo.com>,
        David Woodhouse <dwmw2@infradead.org>,
        "Benjamin Herrenschmidt" <benh@kernel.crashing.org>,
        Jonathan Corbet <corbet@lwn.net>,
        "Rafael J . Wysocki" <rafael@kernel.org>,
        Peter Zilstra <peterz@infradead.org>,
        Andy Lutomirski <luto@kernel.org>,
        Andrew Cooper <Andrew.Cooper3@citrix.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        Martin Pohlack <mpohlack@amazon.de>,
        "Li, Aubrey" <aubrey.li@intel.com>, Ashok Raj <ashok.raj@intel.com>
Subject: Re: [Patch v3 Part2 1/9] x86/microcode: Taint kernel only if
 microcode loading was successful
Message-ID: <Y9qIalgcZmpX6eCR@a4bf019067fa.jf.intel.com>
References: <20230130213955.6046-1-ashok.raj@intel.com>
 <20230130213955.6046-2-ashok.raj@intel.com>
 <Y9kAlKFfdek2rq6g@zn.tnic>
 <Y9lHDWjjnqdletL3@a4bf019067fa.jf.intel.com>
 <Y9l4FdvytLqMc3jm@zn.tnic>
 <Y9mcCxYK/VPB0ATM@a4bf019067fa.jf.intel.com>
 <Y9perwfaheZqAJWl@zn.tnic>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Y9perwfaheZqAJWl@zn.tnic>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?szn7wVyOAUb/qDv+GmfeswtpqY44G7Wa33pGlKxNI1VvTYdrG2naPQQLH3hc?=
 =?us-ascii?Q?IsrOH6/xhjkpKFtUagwwgOSGd1W7ZCf8kvVUT631dPb3M5OLlQ0ROt8nG/V9?=
 =?us-ascii?Q?qRr7bMRO8GyqHmbUOeeLlOdoUVk1ESXEnRkxYOG38nJ/HdTRTVdmaklaMoHN?=
 =?us-ascii?Q?B0SEh2Bz5quXesJcOW+NMe+w+qG165tazDnbMBdCYPX8uU36AjdiibMYVIVB?=
 =?us-ascii?Q?PesdPWXsUge9Sj5I1OJYv+qsWwbopgJc9OxL1CTNylfssVqa3a6UwAXsMyI1?=
 =?us-ascii?Q?Wl4Z1LBf2wE0vb/4ToCWp8p2TAmz+527JBqxzsits447aVuFWEXjSX3UHTMq?=
 =?us-ascii?Q?62uPfG+1ilSevRy532XT306ZYo+RDZEXfhrw7mSPmniWPAQCghHtQ2evV+PH?=
 =?us-ascii?Q?oULoayb3+cqATzMOJ/NBPRCvDhIHtZjgN+BnQskMo4Z48BG4iZaFHbvgccD6?=
 =?us-ascii?Q?zWArV2QISs/gAMTo/iCe1zZhkbgKE8ZyPmWy6JO5FaTJHbLYEqfMwxz5nY9J?=
 =?us-ascii?Q?SO5PsDLhYQrYlE4iBWYaXHaYKvMl0/TW+DI2mnt1Ztndg7aBdCBxeevAhwIJ?=
 =?us-ascii?Q?ON95cm5YUlZCA7I9X2Pbzx4LD4yk6vMppsUzcGb5C5wLbvIgUHPy7ISWuTwx?=
 =?us-ascii?Q?AvfWPKI61cFpbXeKIEAPycgaxZPvvls5fOdFist18LWJnW1vduqkpohAIpXH?=
 =?us-ascii?Q?GPejYIN5L2Ttwul+0e8MfKFQJE8Mqcad7haH5iFvvde2rpUzF38H1gJcdmX9?=
 =?us-ascii?Q?W34sko8QuJau0b9lRmgH+cldd/LPYEEf/lqMQBG00iP4Uovk5oT/T3Vi/mrf?=
 =?us-ascii?Q?s3P6Wi6tkfXCtRkLf1m6KGQDTAW5+GHYeKz+BlLVxCx3H4+ZYyGUpWGD0vBx?=
 =?us-ascii?Q?+8F6g0hhbsqsf6yhuws1OfQqonKipgLOpA/93T1f/+5DLMD8PpZ8PhyVrLo2?=
 =?us-ascii?Q?rybETeRIzvS1gyjV8oOqHcDSdgw2SvdmV3+geLEdUIjNcQI3DsOL9chTxmse?=
 =?us-ascii?Q?+syzO0bmyhrXpDWqVtBxGBcFnWKdfhzhzj8XiWfz2/ZTrUFDEFOFtK47L+nw?=
 =?us-ascii?Q?4rCdMTw+6V52n7Tv0b3c5QJUNrPlFhKubkSKcGB63BGwHw8OphheYWlp1125?=
 =?us-ascii?Q?YM/mzz26LDN6DDGj+Mt9qeLo9UDVL76v9XOoD6Xm+nsDWt6tcbVzS40fVYKz?=
 =?us-ascii?Q?RNNu6qQiLH+ZhnPm/2djrsq27RVbH3dbmfRZujgoEx3eZxvscwwFq+ZfZBoC?=
 =?us-ascii?Q?3T5+bbdyQnyrtiBMYfH7/dMGQUOpGXZ1XqwSgPeJpctKy3zQsBCNVfwEsE++?=
 =?us-ascii?Q?7t9W+O+l3CixS27fWOjavUiqwgoahqowsNgDQpa96tYZgjbtfbhtVPYsBzN1?=
 =?us-ascii?Q?C1S3yCzCNtwzOIq+3wCODDaOB5KFSlcyCjc2Jrh4JNtv3RfpA6Vqk2gU8aQr?=
 =?us-ascii?Q?P8pRhFjdYoj8cF5iZGA/PGLYUn8YUio2QzKW15yEnnCh4qgM7VSRhlA3CDh/?=
 =?us-ascii?Q?eDN8k6bLKy/1OJ0eKlbIU2AXgjTx6BO6mgD7xBBrBQpcLZodVHSWwPN0jAI1?=
 =?us-ascii?Q?V995gDijxTdaJySETjiIfXXh3oKJ/jnfdvSW3GQDt0IHnVszV3c3qNwNqIAs?=
 =?us-ascii?Q?Gg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 007169c6-9f70-43f4-51c5-08db046b04e2
X-MS-Exchange-CrossTenant-AuthSource: SJ1PR11MB6201.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Feb 2023 15:43:09.5366
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 7dO1/ctO4OGzi5CYq9LQsXaST+QBIbKRP4n1u3hVKxodgHqbi8iJUxs8ShkRJHLz7gAyPxrrZPMv4fwEVOTcdQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7336
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Feb 01, 2023 at 01:44:31PM +0100, Borislav Petkov wrote:
> On Tue, Jan 31, 2023 at 02:54:03PM -0800, Ashok Raj wrote:
> > It's not an error, only when request_microcode() returns UCODE_ERROR, should
> > it return -EINVAL,
> 
> So looking at ->request_microcode_fw(), it looks like we return
> UCODE_ERROR when something with parsing the blob has gone wrong. So I
> guess we can return something more fitting here to state that we failed
> while parsing the microcode blob from userspace: it is corrupted,
> truncated, what not.
> 
> Looking at the error codes, this:
> 
> #define ELIBBAD         80      /* Accessing a corrupted shared library */
> 
> seems fitting as it has "corrupted" blob in the definition. EBADF sounds
> fitting too. In any case, it should be a distinct error value which
> hints at what goes wrong.

Perfect!. In the next respin, I can update this to EBADF. 

Later patches when we do minrev checks, or if the current rev is higher
than the one in the directory, what error would be fitting? Currently we do
EINVAL, I can't find a suitable one for that, if you have any good
suggestions that would be awesome.

> 
> > This shouldn't be noisy, but if you think this isn't needed, it can go
> > away.
> 
> I think all this preemptive development - it might make sense so let's
> do it - needs to stop. If there's an *actual* real use and need for it
> sure, but let's issue a printk just because is not one of them.
> 
> > When it fails due to current_rev < min_rev, Isn't it good to add indication
> > to user space that it didn't succeed? Thomas wanted these return codes, so
> > someone scripting can get a status after an attempt to load.
> 
> Return codes: yes. Random, flaky, potentially overwritten in the dmesg
> ring buffer error strings - nope. Soon someone will come along and say,
> "hey, don't touch those printk formats - my tool parses them and it'll
> break if you do." Yeah, right.

No arguments there. 

I'll drop the pr_warn() before taint as you suggest.

Appears we have good direction for patches 1-3. 

Cheers,
Ashok