linux-kernel - Re: [PATCH v3 2/3] x86/alternative: Use a helper function for patching alternatives

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: <20251117134146.GHaRsmGrON5UF7q2P8@fat_crate.local>
Date: Mon, 17 Nov 2025 14:41:46 +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 v3 2/3] x86/alternative: Use a helper function for
 patching alternatives

On Mon, Nov 10, 2025 at 09:23:38AM +0100, Juergen Gross wrote:
> +static void __init_or_module analyze_patch_site(struct patch_site *ps,
> +						struct alt_instr *p, struct alt_instr *end)
> +{
> +	struct alt_instr *r;
> +	u8 buff_sz;
> +	u8 *repl;
> +
> +	/*
> +	 * 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).
> +	 */
> +	ps->instr = instr_va(p);
> +	ps->len = p->instrlen;
> +	for (r = p+1; r < end && instr_va(r) == ps->instr; r++) {
> +		ps->len = max(ps->len, r->instrlen);
> +		p->instrlen = r->instrlen = ps->len;
> +	}
> +
> +	BUG_ON(ps->len > sizeof(ps->buff));
> +	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)) {
> +		memcpy(ps->buff, ps->instr, ps->len);
> +		return;
> +	}
> +
> +	repl = (u8 *)&p->repl_offset + p->repl_offset;
> +	DPRINTK(ALT, "feat: %d*32+%d, old: (%pS (%px) len: %d), repl: (%px, len: %d) flags: 0x%x",
> +		p->cpuid >> 5, p->cpuid & 0x1f,
> +		ps->instr, ps->instr, ps->len,
> +		repl, p->replacementlen, p->flags);
> +
> +	memcpy(ps->buff, repl, p->replacementlen);
> +	buff_sz = p->replacementlen;
> +
> +	if (p->flags & ALT_FLAG_DIRECT_CALL)
> +		buff_sz = alt_replace_call(ps->instr, ps->buff, p);
> +
> +	for (; buff_sz < ps->len; buff_sz++)
> +		ps->buff[buff_sz] = 0x90;
> +
> +	__apply_relocation(ps->buff, ps->instr, ps->len, repl, p->replacementlen);
> +
> +	DUMP_BYTES(ALT, ps->instr, ps->len, "%px:   old_insn: ", ps->instr);
> +	DUMP_BYTES(ALT, repl, p->replacementlen, "%px:   rpl_insn: ", repl);
> +	DUMP_BYTES(ALT, ps->buff, ps->len, "%px: final_insn: ", ps->instr);
> +}

Well, this doesn't quite look like what I suggested: if we have a function
analyze_patch_site() then it should do only that - analyze the patch site
*only* and not do the patching too.

With the point being that struct patch_site should carry the necessary
information between an analyze step and a patching step so that you have
simple functions doing one thing and one thing only - not mix up things.

And your 3rd patch is making things even worse again - simply in a different
way.

So please take enough time to split the functionality:

1. one function does only patch sites analysis. Once it is done, the
   patch_site struct will contain *all* possible information for the next
   function:

2. patch site. This one takes the information gathered by the analysis phase
   and uses it to patch the site, fixup direct calls, apply relocations, dump
   debug info and so on.

Remember: the point here is to make the code simpler and understandable and
manageable. Something that is sorely needed in that area before people start
piling up more fancy shit.

But fancy shit will go in *only* after the code is clean and can take fancy
shit naturally. Not dump it ontop and someone else will mop up after.

Thx.

-- 
Regards/Gruss,
    Boris.

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