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

Message-ID: <dd6d1b4c-b474-4160-b160-5e4fa0805fdc@suse.com>
Date: Mon, 17 Nov 2025 15:45:05 +0100
From: Jürgen Groß <jgross@...e.com>
To: Borislav Petkov <bp@...en8.de>
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 17.11.25 14:41, Borislav Petkov wrote:
> 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.

It doesn't do the patching. That is done in the caller of
analyze_patch_site().

What it does do is looking at one patch site and putting the code to be
patched in into the temporary buffer and then it is printing the debug
info related to the result of the analysis.
> 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.
It is following the same direction:

- analyze the patch site (now all of the alternatives for one location)
- putting the code to be patched into the buffer
- print the debug info related to that patch site

> 
> 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.

The reason I was doing that way was to _really_ have the patching done only
once. This includes the case when the original instruction is kept and just
the nops are being optimized.

I can add one other layer doing the split you are asking for: one for gathering
the information and one for applying relocs and debug printing. But I'd really
like to keep the final patching in apply_alternatives(), as this makes it very
clear where the final patching of the code is done.


Juergen

Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3684 bytes)

Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (496 bytes)