linux-kernel - Re: [PATCH v5 5/7] x86/microcode/intel: Implement staging 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20250910183325.GEaMHEdavbE56NiDUF@fat_crate.local>
Date: Wed, 10 Sep 2025 20:33:25 +0200
From: Borislav Petkov <bp@...en8.de>
To: "Chang S. Bae" <chang.seok.bae@...el.com>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org, tglx@...utronix.de,
	mingo@...hat.com, dave.hansen@...ux.intel.com, chao.gao@...el.com,
	abusse@...zon.de
Subject: Re: [PATCH v5 5/7] x86/microcode/intel: Implement staging handler

On Sat, Aug 23, 2025 at 08:52:08AM -0700, Chang S. Bae wrote:
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index 3ca22457d839..a1b13202330d 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -20,6 +20,8 @@
>  #include <linux/cpu.h>
>  #include <linux/uio.h>
>  #include <linux/mm.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>

You do see those are sorted by header name length in a reverse order, right?

>  
>  #include <asm/cpu_device_id.h>
>  #include <asm/processor.h>
> @@ -33,6 +35,16 @@ static const char ucode_path[] = "kernel/x86/microcode/GenuineIntel.bin";
>  
>  #define UCODE_BSP_LOADED	((struct microcode_intel *)0x1UL)
>  
> +/* Defines for the microcode staging mailbox interface */
> +

^ Superfluous newline.

> +#define MBOX_REG_NUM		4
> +#define MBOX_REG_SIZE		sizeof(u32)
> +
> +#define MBOX_CONTROL_OFFSET	0x0
> +#define MBOX_STATUS_OFFSET	0x4
> +
> +#define MASK_MBOX_CTRL_ABORT	BIT(0)
> +
>  /* Current microcode patch used in early patching on the APs. */
>  static struct microcode_intel *ucode_patch_va __read_mostly;
>  static struct microcode_intel *ucode_patch_late __read_mostly;

...

> +/*
> + * Return PAGE_SIZE, or remaining bytes if this is the final chunk
> + */
> +static inline unsigned int calc_next_chunk_size(unsigned int ucode_len, unsigned int offset)
> +{
> +	return min(PAGE_SIZE, ucode_len - offset);
> +}

That oneliner looks useless - sticking a comment over tne min() and putting it
at the single callsite below is good enough.

> +
> +/*
> + * Update the chunk size and decide whether another chunk can be sent.
> + * This accounts for remaining data and retry limits.
> + */
> +static bool can_send_next_chunk(struct staging_state *ss)
> +{
> +	ss->chunk_size = calc_next_chunk_size(ss->ucode_len, ss->offset);
> +	/*
> +	 * Each microcode image is divided into chunks, each at most
> +	 * one page size. A 10-chunk  image would typically require 10
				   ^^^^

> +	 * transactions.
> +	 *
> +	 * However, the hardware managing the mailbox has limited
> +	 * resources and may not cache the entire image, potentially
> +	 * requesting the same chunk multiple times.
> +	 *
> +	 * To tolerate this behavior, allow up to twice the expected
> +	 * number of transactions (i.e., a 10-chunk image can take up to
> +	 * 20 attempts).

Looks quirky but ok, let's try it in practice first...

> +	 *
> +	 * If the number of attempts exceeds this limit, the hardware is
> +	 * likely stuck and mark the state as timeout.
> +	 */
> +	if (ss->bytes_sent + ss->chunk_size > ss->ucode_len * 2) {
> +		ss->state = UCODE_TIMEOUT;
> +		return false;
> +	}
> +
> +	return true;
> +}

...

>  static enum ucode_state do_stage(u64 mmio_pa)
>  {
> -	pr_debug_once("Staging implementation is pending.\n");
> -	return UCODE_ERROR;
> +	struct staging_state ss = {};
> +	int err;
> +
> +	ss.mmio_base = ioremap(mmio_pa, MBOX_REG_NUM * MBOX_REG_SIZE);
> +	if (WARN_ON_ONCE(!ss.mmio_base))
> +		return UCODE_ERROR;
> +
> +	init_stage(&ss);
> +
> +	/* Perform the staging process while within the retry limit */
> +	while (!staging_is_complete(&ss)) {
> +		/* Send a chunk of microcode each time: */
> +		err = send_data_chunk(&ss, ucode_patch_late);
> +		if (err)
> +			break;
> +		/*
> +		 * Then, ask the hardware which piece of the image it
> +		 * needs next. The same piece may be sent more than once.

If this is part of normal operation, your send-max-2x-the-size heuristic might
fail quickly here. I'd track the number of chunks it wants you to send and
then set a per-chunk limit and when it reaches that limit, then cancel the
transaction. Dunno, let's try the simple scheme first...

> +		 */
> +		err = fetch_next_offset(&ss);
> +		if (err)
> +			break;
> +	}
> +
> +	iounmap(ss.mmio_base);
> +
> +	/*
> +	 * The helpers update ss.state on error. The final state is
> +	 * returned to the caller.
> +	 */
> +	return ss.state;
>  }
>  
>  static void stage_microcode(void)
> -- 
> 2.48.1
> 

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette