From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
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.
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.
Add a common helper to fix filter_cpuid_features() as well as clean up
the open coded cases.
Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Reviewed-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
---
Changelog v11:
- Remove one extra tab after X86_CAP_BUF_SIZE.
- Add Reviewed-by tags from Sohil and Pawan.
Changelog v10:
- Reword the patch message a bit.
- Move x86_cap_name() declaration and X86_CAP_BUF_SIZE define to
asm/cpufeature.h.
- Don't include asm/cpu.h in cpuid-deps.c
- Extend the comment above x86_cap_name to include information on buffer
size that needs to be prepared before calling the function.
- Remove the likely(), unlikely() calls since this is not a hot path.
Changelog v9:
- 16 -> X86_CAP_BUF_SIZE.
- Add comment to the x86_cap_name().
Changelog v8:
- Move x86_cap_name() declaration from linux/cpu.h to the arch/cpu.h.
Include arch/cpu.h in the cpuid-deps.c file instead of linux/cpu.h.
Changelog v7:
- sizeof(buf) -> 16
- Rebase onto 7.01-rc1.
Changelog v6:
- Remove parts of the patch message that are redundant and just copy
what's visible in the diff.
- Redo the helper to use an external char buffer instead of a local
static string.
arch/x86/include/asm/cpufeature.h | 4 ++++
arch/x86/kernel/cpu/common.c | 32 ++++++++++++++++++++++++++-----
arch/x86/kernel/cpu/cpuid-deps.c | 21 +++-----------------
3 files changed, 34 insertions(+), 23 deletions(-)
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
+
+const char *x86_cap_name(unsigned int bit, char *buf);
+
#endif /* defined(__KERNEL__) && !defined(__ASSEMBLER__) */
#endif /* _ASM_X86_CPUFEATURE_H */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 76339e988304..0e318f3d56cb 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -678,6 +678,7 @@ cpuid_dependent_features[] = {
static void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn)
{
const struct cpuid_dependent_feature *df;
+ char feature_buf[X86_CAP_BUF_SIZE];
for (df = cpuid_dependent_features; df->feature; df++) {
@@ -700,7 +701,7 @@ static void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn)
continue;
pr_warn("CPU: CPU feature %s disabled, no CPUID level 0x%x\n",
- x86_cap_flags[df->feature], df->level);
+ x86_cap_name(df->feature, feature_buf), df->level);
}
}
@@ -1637,6 +1638,7 @@ static inline bool parse_set_clear_cpuid(char *arg, bool set)
while (arg) {
bool found __maybe_unused = false;
+ char name_buf[X86_CAP_BUF_SIZE];
unsigned int bit;
opt = strsep(&arg, ",");
@@ -1657,10 +1659,7 @@ static inline bool parse_set_clear_cpuid(char *arg, bool set)
setup_clear_cpu_cap(bit);
}
/* empty-string, i.e., ""-defined feature flags */
- if (!x86_cap_flags[bit])
- pr_cont(" %d:%d\n", bit >> 5, bit & 31);
- else
- pr_cont(" %s\n", x86_cap_flags[bit]);
+ pr_cont(" %s\n", x86_cap_name(bit, name_buf));
taint++;
}
@@ -1983,6 +1982,29 @@ static void generic_identify(struct cpuinfo_x86 *c)
#endif
}
+/*
+ * Return the feature "name" if available, otherwise return the
+ * X86_FEATURE_* numerals to make it easier to identify the feature.
+ * Callers of this function need to pass a char * buffer of size
+ * X86_CAP_BUF_SIZE.
+ */
+const char *x86_cap_name(unsigned int bit, char *buf)
+{
+ unsigned int word = bit >> 5;
+ const char *name = NULL;
+
+ if (word < NCAPINTS)
+ name = x86_cap_flags[bit];
+ else if (word < NCAPINTS + NBUGINTS)
+ name = x86_bug_flags[bit - 32 * NCAPINTS];
+
+ if (name)
+ return name;
+
+ snprintf(buf, X86_CAP_BUF_SIZE, "%u:%u", word, bit & 31);
+ return buf;
+}
+
/*
* This does the hard work of actually picking apart the CPU stuff...
*/
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)
-{
- if (x86_cap_flags[feature])
- return x86_cap_flags[feature];
-
- snprintf(buf, 16, "%d*32+%2d", feature / 32, feature % 32);
-
- return buf;
-}
-
void check_cpufeature_deps(struct cpuinfo_x86 *c)
{
- char feature_buf[16], depends_buf[16];
+ char feature_buf[X86_CAP_BUF_SIZE], depends_buf[X86_CAP_BUF_SIZE];
const struct cpuid_dep *d;
for (d = cpuid_deps; d->feature; d++) {
@@ -185,8 +170,8 @@ void check_cpufeature_deps(struct cpuinfo_x86 *c)
*/
pr_warn_once("x86 CPU feature dependency check failure: CPU%d has '%s' enabled but '%s' disabled. Kernel might be fine, but no guarantees.\n",
smp_processor_id(),
- x86_feature_name(d->feature, feature_buf),
- x86_feature_name(d->depends, depends_buf));
+ x86_cap_name(d->feature, feature_buf),
+ x86_cap_name(d->depends, depends_buf));
}
}
}
--
2.53.0On Fri, Mar 20, 2026 at 12:50:19PM +0000, Maciej Wieczor-Retman wrote:
> From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>
> 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?
> 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.
> Add a common helper to fix filter_cpuid_features() as well as clean up
> the open coded cases.
Fix what?
> 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.
> +
> +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.
> #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?
> -{
> - 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
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@intel.com>
>>
>> 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
On Mon, Mar 23, 2026 at 03:52:04PM +0000, Maciej Wieczor-Retman wrote:
> 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?
Yes, maybe, no...
The point is, when you write commit messages, you should *precisely* explain
what the issue or the non-issue is. What you have now is misleading - it
should say what you just wrote above - that this could *potentially* be
a problem but it isn't a problem now.
> 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?
The use being?
Nothing's stopping someone from slapping a longer name in "" in cpufeatures.h
As long as you select a size and you enforce it somewhere and scream loudly
when someone overflows it, then that's good. Otherwise what's the point of
calling it anything if it is not being enforced?
> 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.
Right, makes sense.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 2026-03-23 at 17:23:24 +0100, Borislav Petkov wrote: >On Mon, Mar 23, 2026 at 03:52:04PM +0000, Maciej Wieczor-Retman wrote: >> 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? > >Yes, maybe, no... > >The point is, when you write commit messages, you should *precisely* explain >what the issue or the non-issue is. What you have now is misleading - it >should say what you just wrote above - that this could *potentially* be >a problem but it isn't a problem now. Okay, you're right, that could've been misleading, I'll rewrite that paragraph. >> 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? > >The use being? > >Nothing's stopping someone from slapping a longer name in "" in cpufeatures.h > >As long as you select a size and you enforce it somewhere and scream loudly >when someone overflows it, then that's good. Otherwise what's the point of >calling it anything if it is not being enforced? Ah sorry, it's a bit of a misunderstanding, the X86_CAP_BUF_SIZE refers only to the value that's needed for a buffer when the word:bit format is returned. Otherwise the const char * is returned from the x86_cap_flags[] array. So I guess the constant name should be different? Maybe X86_CAP_NUM_BUF_SIZE? -- Kind regards Maciej Wieczór-Retman
On Mon, Mar 23, 2026 at 04:58:20PM +0000, Maciej Wieczor-Retman wrote:
> Ah sorry, it's a bit of a misunderstanding, the X86_CAP_BUF_SIZE refers only to
> the value that's needed for a buffer when the word:bit format is returned.
> Otherwise the const char * is returned from the x86_cap_flags[] array.
Yes, I know that. That's why I said:
"But this macro name is generically saying, this is the cap string size. Which
makes it misleading."
> So I guess the constant name should be different? Maybe X86_CAP_NUM_BUF_SIZE?
#define X86_NAMELESS_FEAT_STRLEN
or so.
Your naming must be explicit and denote for what this is used.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 2026-03-23 at 18:51:51 +0100, Borislav Petkov wrote: >On Mon, Mar 23, 2026 at 04:58:20PM +0000, Maciej Wieczor-Retman wrote: >> Ah sorry, it's a bit of a misunderstanding, the X86_CAP_BUF_SIZE refers only to >> the value that's needed for a buffer when the word:bit format is returned. >> Otherwise the const char * is returned from the x86_cap_flags[] array. > >Yes, I know that. That's why I said: > >"But this macro name is generically saying, this is the cap string size. Which >makes it misleading." > >> So I guess the constant name should be different? Maybe X86_CAP_NUM_BUF_SIZE? > >#define X86_NAMELESS_FEAT_STRLEN > >or so. > >Your naming must be explicit and denote for what this is used. > >-- >Regards/Gruss, > Boris. > >https://people.kernel.org/tglx/notes-about-netiquette Thanks! I'll correct it -- Kind regards Maciej Wieczór-Retman
On March 23, 2026 11:11:33 AM PDT, Maciej Wieczor-Retman <m.wieczorretman@pm.me> wrote: >On 2026-03-23 at 18:51:51 +0100, Borislav Petkov wrote: >>On Mon, Mar 23, 2026 at 04:58:20PM +0000, Maciej Wieczor-Retman wrote: >>> Ah sorry, it's a bit of a misunderstanding, the X86_CAP_BUF_SIZE refers only to >>> the value that's needed for a buffer when the word:bit format is returned. >>> Otherwise the const char * is returned from the x86_cap_flags[] array. >> >>Yes, I know that. That's why I said: >> >>"But this macro name is generically saying, this is the cap string size. Which >>makes it misleading." >> >>> So I guess the constant name should be different? Maybe X86_CAP_NUM_BUF_SIZE? >> >>#define X86_NAMELESS_FEAT_STRLEN >> >>or so. >> >>Your naming must be explicit and denote for what this is used. >> >>-- >>Regards/Gruss, >> Boris. >> >>https://people.kernel.org/tglx/notes-about-netiquette > >Thanks! I'll correct it > Call it "BUF" or "BUFLEN". It's not the length of the string, but the length of the buffer for the caller to allocate.
© 2016 - 2026 Red Hat, Inc.