linux-kernel - Re: [PATCH v4 3/3] x86/alternative: Patch a single alternative location only once

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] [day] [month] [year] [list]

Message-ID: <20260102160526.GEaVfsxmpUfNQobRS4@fat_crate.local>
Date: Fri, 2 Jan 2026 17:05:26 +0100
From: Borislav Petkov <bp@...en8.de>
To: Juergen Gross <jgross@...e.com>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH v4 3/3] x86/alternative: Patch a single alternative
 location only once

On Wed, Nov 19, 2025 at 05:04:20PM +0100, Juergen Gross wrote:
> Instead of patching a single location potentially multiple times in
> case of nested ALTERNATIVE()s, do the patching only after having
> evaluated all alt_instr instances for that location.
> 
> This has multiple advantages:
> 
> - In case of replacing an indirect with a direct call using the
>   ALT_FLAG_DIRECT_CALL flag, there is no longer the need to have that
>   instance before any other instances at the same location (the
>   original instruction is needed for finding the target of the direct
>   call).
>   This issue has been hit when trying to do paravirt patching similar
>   to the following:
>     ALTERNATIVE_2(PARAVIRT_CALL,    // indirect call
>                   instr, feature,   // native instruction
>                   ALT_CALL_INSTR, X86_FEATURE_XENPV)  // Xen function
>   In case "feature" was true, "instr" replaced the indirect call. Under
>   Xen PV the patching to have a direct call failed, as the original
>   indirect call was no longer there to find the call target.
> 
> - In case of nested ALTERNATIVE()s there is no intermediate replacement
>   visible. This avoids any problems in case e.g. an interrupt is
>   happening between the single instances and the patched location is
>   used during handling the interrupt.
> 
> Signed-off-by: Juergen Gross <jgross@...e.com>
> ---
> V2:
> - complete rework (Boris Petkov)
> V3:
> - rebase to added patch 2
> ---
>  arch/x86/kernel/alternative.c | 42 +++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 21 deletions(-)

That one would then look like this as I've moved the struct patch_site ps at
the beginning of the loop, where it belongs.

Other than that, yap, looks ok, and it seems to work.

Thx.

---

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 66868ecdebd2..6250c192bb59 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -593,39 +593,39 @@ struct patch_site {
 	u8 len;
 };
 
-static void __init_or_module analyze_patch_site(struct patch_site *ps,
-						struct alt_instr *start,
-						struct alt_instr *end)
+static struct alt_instr * __init_or_module analyze_patch_site(struct patch_site *ps,
+							      struct alt_instr *start,
+							      struct alt_instr *end)
 {
-	struct alt_instr *r;
+	struct alt_instr *p = start;
 
 	ps->instr = instr_va(start);
-	ps->len   = start->instrlen;
 
 	/*
 	 * In case of nested ALTERNATIVE()s the outer alternative might add
 	 * more padding. To ensure consistent patching find the max padding for
 	 * all alt_instr entries for this site (nested alternatives result in
 	 * consecutive entries).
+	 * Find the last alt_instr eligible for patching at the site.
 	 */
-	for (r = start+1; r < end && instr_va(r) == ps->instr; r++) {
-		ps->len = max(ps->len, r->instrlen);
-		start->instrlen = r->instrlen = ps->len;
+	for (; p < end && instr_va(p) == ps->instr; p++) {
+		ps->len = max(ps->len, p->instrlen);
+
+		BUG_ON(p->cpuid >= (NCAPINTS + NBUGINTS) * 32);
+
+		/*
+		 * Patch if either:
+		 * - feature is present
+		 * - feature not present but ALT_FLAG_NOT is set to mean,
+		 *   patch if feature is *NOT* present.
+		 */
+		if (!boot_cpu_has(p->cpuid) != !(p->flags & ALT_FLAG_NOT))
+			ps->alt = p;
 	}
 
 	BUG_ON(ps->len > sizeof(ps->buff));
-	BUG_ON(start->cpuid >= (NCAPINTS + NBUGINTS) * 32);
 
-	/*
-	 * Patch if either:
-	 * - feature is present
-	 * - feature not present but ALT_FLAG_NOT is set to mean,
-	 *   patch if feature is *NOT* present.
-	 */
-	if (!boot_cpu_has(start->cpuid) == !(start->flags & ALT_FLAG_NOT))
-		ps->alt = NULL;
-	else
-		ps->alt = start;
+	return p;
 }
 
 static void __init_or_module prep_patch_site(struct patch_site *ps)
@@ -704,10 +704,14 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 	 * So be careful if you want to change the scan order to any other
 	 * order.
 	 */
-	for (a = start; a < end; a++) {
-		struct patch_site ps;
-
-		analyze_patch_site(&ps, a, end);
+	a = start;
+	while (a < end) {
+		struct patch_site ps = {
+			.alt = NULL,
+			.len = 0
+		};
+
+		a = analyze_patch_site(&ps, a, end);
 		prep_patch_site(&ps);
 		patch_site(&ps);
 	}

-- 
Regards/Gruss,
    Boris.

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