Re: [PATCH v11 2/4] x86/cpu: Check if feature string is non-zero

[Maciej Wieczor-Retman]

On 2026-03-23 at 15:24:07 +0100, Borislav Petkov wrote:
>On Fri, Mar 20, 2026 at 12:50:19PM +0000, Maciej Wieczor-Retman wrote:
>> From: Maciej Wieczor-Retman <maciej.wieczor-retman@xxxxxxxxx>
>>
>> In filter_cpuid_features(), x86_cap_flags[] is read, but it's not verified
>> whether the string is non-zero which could lead to unwanted output.
>
>How can this happen?
>
>That loop in there iterates over cpuid_dependent_features[] and all *three*
>features there have non-zero strings.
>
>What am I missing?

It could be a problem if another entry is added that doesn't have the string
specified? Other spots that return either the string or the word:bit format do
check before if (!x86_cap_name[x]) isn't true. Is it not checked here on
purpose? Or is cpuid_dependent_features[] not expected to grow in the future?

>> In two more places there are open coded paths that try to retrieve a
>> feature string, and if there isn't one, the feature word and bit are
>> returned instead.
>
>Yes, as it should work.

With the above paragraph I was trying to establish that there are more places
that can get unified into one single purpose helper.

>> Add a common helper to fix filter_cpuid_features() as well as clean up
>> the open coded cases.
>
>Fix what?

I assumed the lack of null check on x86_cap_name[x] was not intentional.

>> diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
>> index 3ddc1d33399b..0698a6638463 100644
>> --- a/arch/x86/include/asm/cpufeature.h
>> +++ b/arch/x86/include/asm/cpufeature.h
>> @@ -138,5 +138,9 @@ static __always_inline bool _static_cpu_has(u16 bit)
>>  #define CPU_FEATURE_TYPEVAL		boot_cpu_data.x86_vendor, boot_cpu_data.x86, \
>>  					boot_cpu_data.x86_model
>>
>> +#define X86_CAP_BUF_SIZE		16
>
>Except that you have feature strings which are longer than 16 - look at
>arch/x86/kernel/cpu/capflags.c.
>
>But this macro name is generically saying, this is the cap string size. Which
>makes it misleading.

Right, sorry. Perhaps setting it to 24 would make sense? I think the longest
right now is 19, So there'd be some extra space in case a longer string is added
later?

>> +
>> +const char *x86_cap_name(unsigned int bit, char *buf);
>
>No, certainly NOT exported through a common header which others can use.
>
>arch/x86/kernel/cpu/cpu.h probably which is an internal enough.

Thanks, I'll switch it to that, I wasn't sure which one would be most
appropriate.

>>  #endif /* defined(__KERNEL__) && !defined(__ASSEMBLER__) */
>>  #endif /* _ASM_X86_CPUFEATURE_H */
>
>...
>
>> diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
>> index 146f6f8b0650..5002f496d095 100644
>> --- a/arch/x86/kernel/cpu/cpuid-deps.c
>> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
>> @@ -156,24 +156,9 @@ void setup_clear_cpu_cap(unsigned int feature)
>>  	do_clear_cpu_cap(NULL, feature);
>>  }
>>
>> -/*
>> - * Return the feature "name" if available, otherwise return
>> - * the X86_FEATURE_* numerals to make it easier to identify
>> - * the feature.
>> - */
>> -static const char *x86_feature_name(unsigned int feature, char *buf)
>
>What's wrong with keeping "x86_feature_name" - it is a perfectly fine function
>name - and calling it x86_cap_name? Why don't you just fix that function to
>DTRT as needed?

Sure, I can switch the name to x86_feature_name(). But I assume keeping it in
common.c makes sense? Since it could be used there mostly and at that point it's
more generic and not related to cpuid-deps specifically.

>> -{
>> -	if (x86_cap_flags[feature])
>> -		return x86_cap_flags[feature];
>> -
>> -	snprintf(buf, 16, "%d*32+%2d", feature / 32, feature % 32);
>> -
>> -	return buf;
>> -}
>
>--
>Regards/Gruss,
>    Boris.
>
>https://people.kernel.org/tglx/notes-about-netiquette

-- 
Kind regards
Maciej Wieczór-Retman