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