The use of uninitialised data is undefined behaviour. At -O2 with trivial
examples, both Clang and GCC delete the variable, and in the case of a
function return, the caller gets whatever was stale in %rax prior to the call.
Clang includes -Wuninitialized within -Wall, but GCC only includes it in
-Wextra, which is not used by Xen at this time.
Furthermore, the specific pattern of assigning a variable to itself in its
declaration is only diagnosed by GCC with -Winit-self. Clang does diagnoise
simple forms of this pattern with a plain -Wuninitialized, but it fails to
diagnose the instances in Xen that GCC manages to find.
GCC, with -Wuninitialized and -Winit-self notices:
arch/x86/time.c: In function ‘read_pt_and_tsc’:
arch/x86/time.c:297:14: error: ‘best’ is used uninitialized in this function [-Werror=uninitialized]
297 | uint32_t best = best;
| ^~~~
arch/x86/time.c: In function ‘read_pt_and_tmcct’:
arch/x86/time.c:1022:14: error: ‘best’ is used uninitialized in this function [-Werror=uninitialized]
1022 | uint64_t best = best;
| ^~~~
and both have logic paths where best can be returned while uninitalised. In
both cases, initialise to ~0 like the associated *_min variable which also
gates updating best.
Fixes: 23658e823238 ("x86/time: further improve TSC / CPU freq calibration accuracy")
Fixes: 3f3906b462d5 ("x86/APIC: calibrate against platform timer when possible")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
Full Gitlab:
https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1121715339
---
xen/Makefile | 3 ++-
xen/arch/x86/time.c | 4 ++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/xen/Makefile b/xen/Makefile
index 21832d640225..81861bc41867 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -393,8 +393,9 @@ CFLAGS-$(CONFIG_CC_SPLIT_SECTIONS) += -ffunction-sections -fdata-sections
CFLAGS += -nostdinc -fno-builtin -fno-common
CFLAGS += -Werror -Wredundant-decls -Wwrite-strings -Wno-pointer-arith
-CFLAGS += -Wdeclaration-after-statement
+CFLAGS += -Wdeclaration-after-statement -Wuninitialized
$(call cc-option-add,CFLAGS,CC,-Wvla)
+$(call cc-option-add,CFLAGS,CC,-Winit-self)
CFLAGS += -pipe -D__XEN__ -include $(srctree)/include/xen/config.h
CFLAGS-$(CONFIG_DEBUG_INFO) += -g
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 6d33edd0addc..3fd90e9b78bc 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -294,7 +294,7 @@ static uint32_t __init read_pt_and_tsc(uint64_t *tsc,
const struct platform_timesource *pts)
{
uint64_t tsc_prev = *tsc = rdtsc_ordered(), tsc_min = ~0;
- uint32_t best = best;
+ uint32_t best = ~0;
unsigned int i;
for ( i = 0; ; ++i )
@@ -1019,7 +1019,7 @@ static u64 __init init_platform_timer(void)
static uint64_t __init read_pt_and_tmcct(uint32_t *tmcct)
{
uint32_t tmcct_prev = *tmcct = apic_tmcct_read(), tmcct_min = ~0;
- uint64_t best = best;
+ uint64_t best = ~0;
unsigned int i;
for ( i = 0; ; ++i )
--
2.30.2
examples, both Clang and GCC delete the variable, and in the case of a
function return, the caller gets whatever was stale in %rax prior to the call.
Clang includes -Wuninitialized within -Wall, but GCC only includes it in
-Wextra, which is not used by Xen at this time.
Furthermore, the specific pattern of assigning a variable to itself in its
declaration is only diagnosed by GCC with -Winit-self. Clang does diagnoise
simple forms of this pattern with a plain -Wuninitialized, but it fails to
diagnose the instances in Xen that GCC manages to find.
GCC, with -Wuninitialized and -Winit-self notices:
arch/x86/time.c: In function ‘read_pt_and_tsc’:
arch/x86/time.c:297:14: error: ‘best’ is used uninitialized in this function [-Werror=uninitialized]
297 | uint32_t best = best;
| ^~~~
arch/x86/time.c: In function ‘read_pt_and_tmcct’:
arch/x86/time.c:1022:14: error: ‘best’ is used uninitialized in this function [-Werror=uninitialized]
1022 | uint64_t best = best;
| ^~~~
and both have logic paths where best can be returned while uninitalised. In
both cases, initialise to ~0 like the associated *_min variable which also
gates updating best.
Fixes: 23658e823238 ("x86/time: further improve TSC / CPU freq calibration accuracy")
Fixes: 3f3906b462d5 ("x86/APIC: calibrate against platform timer when possible")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
Full Gitlab:
https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1121715339
---
xen/Makefile | 3 ++-
xen/arch/x86/time.c | 4 ++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/xen/Makefile b/xen/Makefile
index 21832d640225..81861bc41867 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -393,8 +393,9 @@ CFLAGS-$(CONFIG_CC_SPLIT_SECTIONS) += -ffunction-sections -fdata-sections
CFLAGS += -nostdinc -fno-builtin -fno-common
CFLAGS += -Werror -Wredundant-decls -Wwrite-strings -Wno-pointer-arith
-CFLAGS += -Wdeclaration-after-statement
+CFLAGS += -Wdeclaration-after-statement -Wuninitialized
$(call cc-option-add,CFLAGS,CC,-Wvla)
+$(call cc-option-add,CFLAGS,CC,-Winit-self)
CFLAGS += -pipe -D__XEN__ -include $(srctree)/include/xen/config.h
CFLAGS-$(CONFIG_DEBUG_INFO) += -g
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 6d33edd0addc..3fd90e9b78bc 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -294,7 +294,7 @@ static uint32_t __init read_pt_and_tsc(uint64_t *tsc,
const struct platform_timesource *pts)
{
uint64_t tsc_prev = *tsc = rdtsc_ordered(), tsc_min = ~0;
- uint32_t best = best;
+ uint32_t best = ~0;
unsigned int i;
for ( i = 0; ; ++i )
@@ -1019,7 +1019,7 @@ static u64 __init init_platform_timer(void)
static uint64_t __init read_pt_and_tmcct(uint32_t *tmcct)
{
uint32_t tmcct_prev = *tmcct = apic_tmcct_read(), tmcct_min = ~0;
- uint64_t best = best;
+ uint64_t best = ~0;
unsigned int i;
for ( i = 0; ; ++i )
--
2.30.2