diff mbox series

[CPU,v1] cpuid: initialize cpuinfo with boot_cpu_data

Message ID 20220211072327.1213-1-nmanthey@amazon.de (mailing list archive)
State Superseded
Headers show
Series [CPU,v1] cpuid: initialize cpuinfo with boot_cpu_data | expand

Commit Message

Norbert Manthey Feb. 11, 2022, 7:23 a.m. UTC
When re-identifying CPU data, we might use uninitialized data when
checking for the cache line property to adapt the cache
alignment. The data that depends on this uninitialized read is
currently not forwarded.

To avoid problems in the future, initialize the data cpuinfo
structure before re-identifying the CPU again.

The trace to hit the uninitialized read reported by Coverity is:

bool recheck_cpu_features(unsigned int cpu)
...
    struct cpuinfo_x86 c;
    ...
    identify_cpu(&c);

void identify_cpu(struct cpuinfo_x86 *c)
...
    generic_identify(c)

static void generic_identify(struct cpuinfo_x86 *c)
...
        if (this_cpu->c_early_init)
                this_cpu->c_early_init(c); // which is early_init_intel

static void early_init_intel(struct cpuinfo_x86 *c)
...
    if (c->x86 == 15 && c->x86_cache_alignment == 64)
        c->x86_cache_alignment = 128;

This bug was discovered and resolved using Coverity Static Analysis
Security Testing (SAST) by Synopsys, Inc.

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>

---
 xen/arch/x86/cpuid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Roger Pau Monné Feb. 11, 2022, 9:02 a.m. UTC | #1
On Fri, Feb 11, 2022 at 08:23:27AM +0100, Norbert Manthey wrote:
> When re-identifying CPU data, we might use uninitialized data when
> checking for the cache line property to adapt the cache
> alignment. The data that depends on this uninitialized read is
> currently not forwarded.
> 
> To avoid problems in the future, initialize the data cpuinfo
> structure before re-identifying the CPU again.
> 
> The trace to hit the uninitialized read reported by Coverity is:
> 
> bool recheck_cpu_features(unsigned int cpu)
> ...
>     struct cpuinfo_x86 c;
>     ...
>     identify_cpu(&c);
> 
> void identify_cpu(struct cpuinfo_x86 *c)
> ...
>     generic_identify(c)
> 
> static void generic_identify(struct cpuinfo_x86 *c)
> ...

Would it be more appropriate for generic_identify to also set
x86_cache_alignment like it's done in early_cpu_init?

generic_identify already re-fetches a bunch of stuff that's also
set by early_cpu_init for the BSP.

Thanks, Roger.
Jan Beulich Feb. 11, 2022, 10:32 a.m. UTC | #2
On 11.02.2022 10:02, Roger Pau Monné wrote:
> On Fri, Feb 11, 2022 at 08:23:27AM +0100, Norbert Manthey wrote:
>> When re-identifying CPU data, we might use uninitialized data when
>> checking for the cache line property to adapt the cache
>> alignment. The data that depends on this uninitialized read is
>> currently not forwarded.
>>
>> To avoid problems in the future, initialize the data cpuinfo
>> structure before re-identifying the CPU again.
>>
>> The trace to hit the uninitialized read reported by Coverity is:
>>
>> bool recheck_cpu_features(unsigned int cpu)
>> ...
>>     struct cpuinfo_x86 c;
>>     ...
>>     identify_cpu(&c);
>>
>> void identify_cpu(struct cpuinfo_x86 *c)
>> ...
>>     generic_identify(c)
>>
>> static void generic_identify(struct cpuinfo_x86 *c)
>> ...
> 
> Would it be more appropriate for generic_identify to also set
> x86_cache_alignment like it's done in early_cpu_init?
> 
> generic_identify already re-fetches a bunch of stuff that's also
> set by early_cpu_init for the BSP.

This would be an option, but how sure are you that there isn't
(going to be) another field with similar properties? We better
wouldn't require _everything_ to be re-filled in generic_identify().

Jan
Jan Beulich Feb. 11, 2022, 10:34 a.m. UTC | #3
On 11.02.2022 08:23, Norbert Manthey wrote:
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -609,7 +609,7 @@ void __init init_guest_cpuid(void)
>  bool recheck_cpu_features(unsigned int cpu)
>  {
>      bool okay = true;
> -    struct cpuinfo_x86 c;
> +    struct cpuinfo_x86 c = boot_cpu_data;
>      const struct cpuinfo_x86 *bsp = &boot_cpu_data;
>      unsigned int i;

While I agree with the need to initialize the local variable, I
don't think it should be pre-seeded with previous indentification
results: This could end up hiding bugs. Instead I'd see it simply
be zero-filled.

Jan
Roger Pau Monné Feb. 11, 2022, 10:47 a.m. UTC | #4
On Fri, Feb 11, 2022 at 11:32:45AM +0100, Jan Beulich wrote:
> On 11.02.2022 10:02, Roger Pau Monné wrote:
> > On Fri, Feb 11, 2022 at 08:23:27AM +0100, Norbert Manthey wrote:
> >> When re-identifying CPU data, we might use uninitialized data when
> >> checking for the cache line property to adapt the cache
> >> alignment. The data that depends on this uninitialized read is
> >> currently not forwarded.
> >>
> >> To avoid problems in the future, initialize the data cpuinfo
> >> structure before re-identifying the CPU again.
> >>
> >> The trace to hit the uninitialized read reported by Coverity is:
> >>
> >> bool recheck_cpu_features(unsigned int cpu)
> >> ...
> >>     struct cpuinfo_x86 c;
> >>     ...
> >>     identify_cpu(&c);
> >>
> >> void identify_cpu(struct cpuinfo_x86 *c)
> >> ...
> >>     generic_identify(c)
> >>
> >> static void generic_identify(struct cpuinfo_x86 *c)
> >> ...
> > 
> > Would it be more appropriate for generic_identify to also set
> > x86_cache_alignment like it's done in early_cpu_init?
> > 
> > generic_identify already re-fetches a bunch of stuff that's also
> > set by early_cpu_init for the BSP.
> 
> This would be an option, but how sure are you that there isn't
> (going to be) another field with similar properties? We better
> wouldn't require _everything_ to be re-filled in generic_identify().

So you think generic_identify should call into early_cpu_init, or even
split the cpuinfo_x86 filling done in early_cpu_init into a non-init
function that could be called by both generic_identify and
early_cpu_init?

Thanks, Roger.
Jan Beulich Feb. 11, 2022, 10:50 a.m. UTC | #5
On 11.02.2022 11:47, Roger Pau Monné wrote:
> On Fri, Feb 11, 2022 at 11:32:45AM +0100, Jan Beulich wrote:
>> On 11.02.2022 10:02, Roger Pau Monné wrote:
>>> On Fri, Feb 11, 2022 at 08:23:27AM +0100, Norbert Manthey wrote:
>>>> When re-identifying CPU data, we might use uninitialized data when
>>>> checking for the cache line property to adapt the cache
>>>> alignment. The data that depends on this uninitialized read is
>>>> currently not forwarded.
>>>>
>>>> To avoid problems in the future, initialize the data cpuinfo
>>>> structure before re-identifying the CPU again.
>>>>
>>>> The trace to hit the uninitialized read reported by Coverity is:
>>>>
>>>> bool recheck_cpu_features(unsigned int cpu)
>>>> ...
>>>>     struct cpuinfo_x86 c;
>>>>     ...
>>>>     identify_cpu(&c);
>>>>
>>>> void identify_cpu(struct cpuinfo_x86 *c)
>>>> ...
>>>>     generic_identify(c)
>>>>
>>>> static void generic_identify(struct cpuinfo_x86 *c)
>>>> ...
>>>
>>> Would it be more appropriate for generic_identify to also set
>>> x86_cache_alignment like it's done in early_cpu_init?
>>>
>>> generic_identify already re-fetches a bunch of stuff that's also
>>> set by early_cpu_init for the BSP.
>>
>> This would be an option, but how sure are you that there isn't
>> (going to be) another field with similar properties? We better
>> wouldn't require _everything_ to be re-filled in generic_identify().
> 
> So you think generic_identify should call into early_cpu_init, or even
> split the cpuinfo_x86 filling done in early_cpu_init into a non-init
> function that could be called by both generic_identify and
> early_cpu_init?

No, I think it is quite fine for this to be a two-step process. In
fact I was hoping to eliminate some of the remaining redundancy
where possible. recheck_cpu_features(), after all, cares about just
the feature flags, so doesn't require things like cache line
alignment to be filled in.

Jan
Roger Pau Monné Feb. 11, 2022, 11:19 a.m. UTC | #6
On Fri, Feb 11, 2022 at 11:50:46AM +0100, Jan Beulich wrote:
> On 11.02.2022 11:47, Roger Pau Monné wrote:
> > On Fri, Feb 11, 2022 at 11:32:45AM +0100, Jan Beulich wrote:
> >> On 11.02.2022 10:02, Roger Pau Monné wrote:
> >>> On Fri, Feb 11, 2022 at 08:23:27AM +0100, Norbert Manthey wrote:
> >>>> When re-identifying CPU data, we might use uninitialized data when
> >>>> checking for the cache line property to adapt the cache
> >>>> alignment. The data that depends on this uninitialized read is
> >>>> currently not forwarded.
> >>>>
> >>>> To avoid problems in the future, initialize the data cpuinfo
> >>>> structure before re-identifying the CPU again.
> >>>>
> >>>> The trace to hit the uninitialized read reported by Coverity is:
> >>>>
> >>>> bool recheck_cpu_features(unsigned int cpu)
> >>>> ...
> >>>>     struct cpuinfo_x86 c;
> >>>>     ...
> >>>>     identify_cpu(&c);
> >>>>
> >>>> void identify_cpu(struct cpuinfo_x86 *c)
> >>>> ...
> >>>>     generic_identify(c)
> >>>>
> >>>> static void generic_identify(struct cpuinfo_x86 *c)
> >>>> ...
> >>>
> >>> Would it be more appropriate for generic_identify to also set
> >>> x86_cache_alignment like it's done in early_cpu_init?
> >>>
> >>> generic_identify already re-fetches a bunch of stuff that's also
> >>> set by early_cpu_init for the BSP.
> >>
> >> This would be an option, but how sure are you that there isn't
> >> (going to be) another field with similar properties? We better
> >> wouldn't require _everything_ to be re-filled in generic_identify().
> > 
> > So you think generic_identify should call into early_cpu_init, or even
> > split the cpuinfo_x86 filling done in early_cpu_init into a non-init
> > function that could be called by both generic_identify and
> > early_cpu_init?
> 
> No, I think it is quite fine for this to be a two-step process.

But it's not a two step process for all CPUs. It's a two step process
for the BSP, that will get it's cpuinfo filled by early_cpu_init
first, and then by identify_cpu. OTHO APs will only get the
information filled by identify_cpu.

Maybe APs don't care about having x86_cache_alignment correctly set?

Thanks, Roger.
Jan Beulich Feb. 11, 2022, 11:42 a.m. UTC | #7
On 11.02.2022 12:19, Roger Pau Monné wrote:
> On Fri, Feb 11, 2022 at 11:50:46AM +0100, Jan Beulich wrote:
>> On 11.02.2022 11:47, Roger Pau Monné wrote:
>>> On Fri, Feb 11, 2022 at 11:32:45AM +0100, Jan Beulich wrote:
>>>> On 11.02.2022 10:02, Roger Pau Monné wrote:
>>>>> On Fri, Feb 11, 2022 at 08:23:27AM +0100, Norbert Manthey wrote:
>>>>>> When re-identifying CPU data, we might use uninitialized data when
>>>>>> checking for the cache line property to adapt the cache
>>>>>> alignment. The data that depends on this uninitialized read is
>>>>>> currently not forwarded.
>>>>>>
>>>>>> To avoid problems in the future, initialize the data cpuinfo
>>>>>> structure before re-identifying the CPU again.
>>>>>>
>>>>>> The trace to hit the uninitialized read reported by Coverity is:
>>>>>>
>>>>>> bool recheck_cpu_features(unsigned int cpu)
>>>>>> ...
>>>>>>     struct cpuinfo_x86 c;
>>>>>>     ...
>>>>>>     identify_cpu(&c);
>>>>>>
>>>>>> void identify_cpu(struct cpuinfo_x86 *c)
>>>>>> ...
>>>>>>     generic_identify(c)
>>>>>>
>>>>>> static void generic_identify(struct cpuinfo_x86 *c)
>>>>>> ...
>>>>>
>>>>> Would it be more appropriate for generic_identify to also set
>>>>> x86_cache_alignment like it's done in early_cpu_init?
>>>>>
>>>>> generic_identify already re-fetches a bunch of stuff that's also
>>>>> set by early_cpu_init for the BSP.
>>>>
>>>> This would be an option, but how sure are you that there isn't
>>>> (going to be) another field with similar properties? We better
>>>> wouldn't require _everything_ to be re-filled in generic_identify().
>>>
>>> So you think generic_identify should call into early_cpu_init, or even
>>> split the cpuinfo_x86 filling done in early_cpu_init into a non-init
>>> function that could be called by both generic_identify and
>>> early_cpu_init?
>>
>> No, I think it is quite fine for this to be a two-step process.
> 
> But it's not a two step process for all CPUs. It's a two step process
> for the BSP, that will get it's cpuinfo filled by early_cpu_init
> first, and then by identify_cpu. OTHO APs will only get the
> information filled by identify_cpu.
> 
> Maybe APs don't care about having x86_cache_alignment correctly set?

They do care, and the field also isn't left uninitialized. See
initialize_cpu_data(). Like in many other places we assume full
symmetry among processors here.

Jan
Norbert Manthey Feb. 11, 2022, 11:58 a.m. UTC | #8
On 2/11/22 11:34, Jan Beulich wrote:
> On 11.02.2022 08:23, Norbert Manthey wrote:
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -609,7 +609,7 @@ void __init init_guest_cpuid(void)
>>  bool recheck_cpu_features(unsigned int cpu)
>>  {
>>      bool okay = true;
>> -    struct cpuinfo_x86 c;
>> +    struct cpuinfo_x86 c = boot_cpu_data;
>>      const struct cpuinfo_x86 *bsp = &boot_cpu_data;
>>      unsigned int i;
> While I agree with the need to initialize the local variable, I
> don't think it should be pre-seeded with previous indentification
> results: This could end up hiding bugs. Instead I'd see it simply
> be zero-filled.

That works for me as well, I'll send a rev-2 accordingly.

Norbert




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
Roger Pau Monné Feb. 11, 2022, 11:58 a.m. UTC | #9
On Fri, Feb 11, 2022 at 12:42:11PM +0100, Jan Beulich wrote:
> On 11.02.2022 12:19, Roger Pau Monné wrote:
> > On Fri, Feb 11, 2022 at 11:50:46AM +0100, Jan Beulich wrote:
> >> On 11.02.2022 11:47, Roger Pau Monné wrote:
> >>> On Fri, Feb 11, 2022 at 11:32:45AM +0100, Jan Beulich wrote:
> >>>> On 11.02.2022 10:02, Roger Pau Monné wrote:
> >>>>> On Fri, Feb 11, 2022 at 08:23:27AM +0100, Norbert Manthey wrote:
> >>>>>> When re-identifying CPU data, we might use uninitialized data when
> >>>>>> checking for the cache line property to adapt the cache
> >>>>>> alignment. The data that depends on this uninitialized read is
> >>>>>> currently not forwarded.
> >>>>>>
> >>>>>> To avoid problems in the future, initialize the data cpuinfo
> >>>>>> structure before re-identifying the CPU again.
> >>>>>>
> >>>>>> The trace to hit the uninitialized read reported by Coverity is:
> >>>>>>
> >>>>>> bool recheck_cpu_features(unsigned int cpu)
> >>>>>> ...
> >>>>>>     struct cpuinfo_x86 c;
> >>>>>>     ...
> >>>>>>     identify_cpu(&c);
> >>>>>>
> >>>>>> void identify_cpu(struct cpuinfo_x86 *c)
> >>>>>> ...
> >>>>>>     generic_identify(c)
> >>>>>>
> >>>>>> static void generic_identify(struct cpuinfo_x86 *c)
> >>>>>> ...
> >>>>>
> >>>>> Would it be more appropriate for generic_identify to also set
> >>>>> x86_cache_alignment like it's done in early_cpu_init?
> >>>>>
> >>>>> generic_identify already re-fetches a bunch of stuff that's also
> >>>>> set by early_cpu_init for the BSP.
> >>>>
> >>>> This would be an option, but how sure are you that there isn't
> >>>> (going to be) another field with similar properties? We better
> >>>> wouldn't require _everything_ to be re-filled in generic_identify().
> >>>
> >>> So you think generic_identify should call into early_cpu_init, or even
> >>> split the cpuinfo_x86 filling done in early_cpu_init into a non-init
> >>> function that could be called by both generic_identify and
> >>> early_cpu_init?
> >>
> >> No, I think it is quite fine for this to be a two-step process.
> > 
> > But it's not a two step process for all CPUs. It's a two step process
> > for the BSP, that will get it's cpuinfo filled by early_cpu_init
> > first, and then by identify_cpu. OTHO APs will only get the
> > information filled by identify_cpu.
> > 
> > Maybe APs don't care about having x86_cache_alignment correctly set?
> 
> They do care, and the field also isn't left uninitialized. See
> initialize_cpu_data(). Like in many other places we assume full
> symmetry among processors here.

Thanks, I did miss that part. Then I think it's fine to initialize the
struct in recheck_cpu_features to zero. That's likely to make it more
obvious if we somehow miss to update certain featuresets? (as they
would be empty instead of inheriting from boot CPU data).

Thanks, Roger.
diff mbox series

Patch

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -609,7 +609,7 @@  void __init init_guest_cpuid(void)
 bool recheck_cpu_features(unsigned int cpu)
 {
     bool okay = true;
-    struct cpuinfo_x86 c;
+    struct cpuinfo_x86 c = boot_cpu_data;
     const struct cpuinfo_x86 *bsp = &boot_cpu_data;
     unsigned int i;