[v1] cpuid: initialize cpuinfo with boot_cpu_data

[PATCH CPU v1] cpuid: initialize cpuinfo with boot_cpu_data

Posted by Norbert Manthey 2 years, 2 months ago

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(-)

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;
 
-- 
2.17.1




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

Re: [PATCH CPU v1] cpuid: initialize cpuinfo with boot_cpu_data

Posted by Roger Pau Monné 2 years, 2 months ago

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.

Re: [PATCH CPU v1] cpuid: initialize cpuinfo with boot_cpu_data

Posted by Jan Beulich 2 years, 2 months ago

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

Re: [PATCH CPU v1] cpuid: initialize cpuinfo with boot_cpu_data

Posted by Roger Pau Monné 2 years, 2 months ago

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.

Re: [PATCH CPU v1] cpuid: initialize cpuinfo with boot_cpu_data

Posted by Jan Beulich 2 years, 2 months ago

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

Re: [PATCH CPU v1] cpuid: initialize cpuinfo with boot_cpu_data

Posted by Roger Pau Monné 2 years, 2 months ago

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.

Re: [PATCH CPU v1] cpuid: initialize cpuinfo with boot_cpu_data

Posted by Jan Beulich 2 years, 2 months ago

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

Re: [PATCH CPU v1] cpuid: initialize cpuinfo with boot_cpu_data

Posted by Roger Pau Monné 2 years, 2 months ago

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.

Re: [PATCH CPU v1] cpuid: initialize cpuinfo with boot_cpu_data

Posted by Jan Beulich 2 years, 2 months ago

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

Re: [PATCH CPU v1] cpuid: initialize cpuinfo with boot_cpu_data

Posted by Norbert Manthey 2 years, 2 months ago

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