linux-kernel - Re: [tip: x86/core] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <4fd058ac-3af5-4778-842c-9a185d828c9d@suse.com>
Date: Tue, 14 Oct 2025 16:08:47 +0200
From: Jürgen Groß <jgross@...e.com>
To: Borislav Petkov <bp@...en8.de>, linux-kernel@...r.kernel.org
Cc: linux-tip-commits@...r.kernel.org,
 "Peter Zijlstra (Intel)" <peterz@...radead.org>, x86@...nel.org
Subject: Re: [tip: x86/core] x86/alternative: Patch a single alternative
 location only once

On 14.10.25 14:59, Borislav Petkov wrote:
> On Tue, Oct 14, 2025 at 08:42:34AM -0000, tip-bot2 for Juergen Gross wrote:
>> @@ -648,6 +648,8 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
>>   	u8 insn_buff[MAX_PATCH_LEN];
>>   	u8 *instr;
>>   	struct alt_instr *a, *b;
>> +	unsigned int instances = 0;
>> +	bool patched = false;
> 
> Except that we have the reverse fir tree rule in tip for function-local vars.

Okay.

>> @@ -692,14 +698,19 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
>>   		 * - feature not present but ALT_FLAG_NOT is set to mean,
>>   		 *   patch if feature is *NOT* present.
>>   		 */
>> -		if (!boot_cpu_has(a->cpuid) == !(a->flags & ALT_FLAG_NOT)) {
>> -			memcpy(insn_buff, instr, a->instrlen);
>> -			optimize_nops(instr, insn_buff, a->instrlen);
>> -		} else {
>> +		if (!boot_cpu_has(a->cpuid) != !(a->flags & ALT_FLAG_NOT)) {
>>   			apply_one_alternative(instr, insn_buff, a);
>> +			patched = true;
>>   		}
>>   
>> -		text_poke_early(instr, insn_buff, a->instrlen);
>> +		instances--;
>> +		if (!instances) {
>> +			if (!patched) {
> 
> I don't see how this is making this code better - this is slowly turning into
> an unreadable mess with those magic "instances" and "patched".
> 
> And frankly, the justification for this patch is also meh: an interrupt might
> use the location?!? If this is a real issue then we better disable IRQs around
> it. But not make the code yucky.

For one it is not the only justification.

And just for the record: NMI handling is using ALTERNATIVE_2, so you
can't just "disable interrupts" and be sure it will work.


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)