| Message ID | 20210322160253.4032422-3-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | treewide: address gcc-11 -Wstringop-overread warnings | expand |
* Arnd Bergmann <arnd@kernel.org> wrote: > From: Arnd Bergmann <arnd@arndb.de> > > gcc-11 warns about using string operations on pointers that are > defined at compile time as offsets from a NULL pointer. Unfortunately > that also happens on the result of fix_to_virt(), which is a > compile-time constant for a constantn input: > > arch/x86/kernel/tboot.c: In function 'tboot_probe': > arch/x86/kernel/tboot.c:70:13: error: '__builtin_memcmp_eq' specified bound 16 exceeds source size 0 [-Werror=stringop-overread] > 70 | if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) { > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > I hope this can get addressed in gcc-11 before the release. > > As a workaround, split up the tboot_probe() function in two halves > to separate the pointer generation from the usage. This is a bit > ugly, and hopefully gcc understands that the code is actually correct > before it learns to peek into the noinline function. > > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578 > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > arch/x86/kernel/tboot.c | 44 ++++++++++++++++++++++++----------------- > 1 file changed, 26 insertions(+), 18 deletions(-) > > diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c > index 4c09ba110204..f9af561c3cd4 100644 > --- a/arch/x86/kernel/tboot.c > +++ b/arch/x86/kernel/tboot.c > @@ -49,6 +49,30 @@ bool tboot_enabled(void) > return tboot != NULL; > } > > +/* noinline to prevent gcc from warning about dereferencing constant fixaddr */ > +static noinline __init bool check_tboot_version(void) > +{ > + if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) { > + pr_warn("tboot at 0x%llx is invalid\n", boot_params.tboot_addr); > + return false; > + } > + > + if (tboot->version < 5) { > + pr_warn("tboot version is invalid: %u\n", tboot->version); > + return false; > + } > + > + pr_info("found shared page at phys addr 0x%llx:\n", > + boot_params.tboot_addr); > + pr_debug("version: %d\n", tboot->version); > + pr_debug("log_addr: 0x%08x\n", tboot->log_addr); > + pr_debug("shutdown_entry: 0x%x\n", tboot->shutdown_entry); > + pr_debug("tboot_base: 0x%08x\n", tboot->tboot_base); > + pr_debug("tboot_size: 0x%x\n", tboot->tboot_size); > + > + return true; > +} > + > void __init tboot_probe(void) > { > /* Look for valid page-aligned address for shared page. */ > @@ -66,25 +90,9 @@ void __init tboot_probe(void) > > /* Map and check for tboot UUID. */ > set_fixmap(FIX_TBOOT_BASE, boot_params.tboot_addr); > - tboot = (struct tboot *)fix_to_virt(FIX_TBOOT_BASE); > - if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) { > - pr_warn("tboot at 0x%llx is invalid\n", boot_params.tboot_addr); > + tboot = (void *)fix_to_virt(FIX_TBOOT_BASE); > + if (!check_tboot_version()) > tboot = NULL; > - return; > - } > - if (tboot->version < 5) { > - pr_warn("tboot version is invalid: %u\n", tboot->version); > - tboot = NULL; > - return; > - } > - > - pr_info("found shared page at phys addr 0x%llx:\n", > - boot_params.tboot_addr); > - pr_debug("version: %d\n", tboot->version); > - pr_debug("log_addr: 0x%08x\n", tboot->log_addr); > - pr_debug("shutdown_entry: 0x%x\n", tboot->shutdown_entry); > - pr_debug("tboot_base: 0x%08x\n", tboot->tboot_base); > - pr_debug("tboot_size: 0x%x\n", tboot->tboot_size); This is indeed rather ugly - and the other patch that removes a debug check seems counterproductive as well. Do we know how many genuine bugs -Wstringop-overread-warning has caught or is about to catch? I.e. the real workaround might be to turn off the -Wstringop-overread-warning, until GCC-11 gets fixed? Thanks, Ingo
On Mon, Mar 22, 2021 at 9:29 PM Ingo Molnar <mingo@kernel.org> wrote: > * Arnd Bergmann <arnd@kernel.org> wrote: > > From: Arnd Bergmann <arnd@arndb.de> > This is indeed rather ugly - and the other patch that removes a debug > check seems counterproductive as well. > > Do we know how many genuine bugs -Wstringop-overread-warning has > caught or is about to catch? > > I.e. the real workaround might be to turn off the -Wstringop-overread-warning, > until GCC-11 gets fixed? See the [PATCH 0/11] message. The last two patches in the series are for code that I suspect may be broken, the others are basically all false positives. As gcc-11 is not released yet, I don't think we have to apply any of the patches or disable the warning at the moment, but I posted all the patches to get a better understanding on which of them should be addressed in the kernel vs gcc. Arnd
On 3/22/21 2:29 PM, Ingo Molnar wrote: > > * Arnd Bergmann <arnd@kernel.org> wrote: > >> From: Arnd Bergmann <arnd@arndb.de> >> >> gcc-11 warns about using string operations on pointers that are >> defined at compile time as offsets from a NULL pointer. Unfortunately >> that also happens on the result of fix_to_virt(), which is a >> compile-time constant for a constantn input: >> >> arch/x86/kernel/tboot.c: In function 'tboot_probe': >> arch/x86/kernel/tboot.c:70:13: error: '__builtin_memcmp_eq' specified bound 16 exceeds source size 0 [-Werror=stringop-overread] >> 70 | if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) { >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> >> I hope this can get addressed in gcc-11 before the release. >> >> As a workaround, split up the tboot_probe() function in two halves >> to separate the pointer generation from the usage. This is a bit >> ugly, and hopefully gcc understands that the code is actually correct >> before it learns to peek into the noinline function. >> >> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578 >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> --- >> arch/x86/kernel/tboot.c | 44 ++++++++++++++++++++++++----------------- >> 1 file changed, 26 insertions(+), 18 deletions(-) >> >> diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c >> index 4c09ba110204..f9af561c3cd4 100644 >> --- a/arch/x86/kernel/tboot.c >> +++ b/arch/x86/kernel/tboot.c >> @@ -49,6 +49,30 @@ bool tboot_enabled(void) >> return tboot != NULL; >> } >> >> +/* noinline to prevent gcc from warning about dereferencing constant fixaddr */ >> +static noinline __init bool check_tboot_version(void) >> +{ >> + if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) { >> + pr_warn("tboot at 0x%llx is invalid\n", boot_params.tboot_addr); >> + return false; >> + } >> + >> + if (tboot->version < 5) { >> + pr_warn("tboot version is invalid: %u\n", tboot->version); >> + return false; >> + } >> + >> + pr_info("found shared page at phys addr 0x%llx:\n", >> + boot_params.tboot_addr); >> + pr_debug("version: %d\n", tboot->version); >> + pr_debug("log_addr: 0x%08x\n", tboot->log_addr); >> + pr_debug("shutdown_entry: 0x%x\n", tboot->shutdown_entry); >> + pr_debug("tboot_base: 0x%08x\n", tboot->tboot_base); >> + pr_debug("tboot_size: 0x%x\n", tboot->tboot_size); >> + >> + return true; >> +} >> + >> void __init tboot_probe(void) >> { >> /* Look for valid page-aligned address for shared page. */ >> @@ -66,25 +90,9 @@ void __init tboot_probe(void) >> >> /* Map and check for tboot UUID. */ >> set_fixmap(FIX_TBOOT_BASE, boot_params.tboot_addr); >> - tboot = (struct tboot *)fix_to_virt(FIX_TBOOT_BASE); >> - if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) { >> - pr_warn("tboot at 0x%llx is invalid\n", boot_params.tboot_addr); >> + tboot = (void *)fix_to_virt(FIX_TBOOT_BASE); >> + if (!check_tboot_version()) >> tboot = NULL; >> - return; >> - } >> - if (tboot->version < 5) { >> - pr_warn("tboot version is invalid: %u\n", tboot->version); >> - tboot = NULL; >> - return; >> - } >> - >> - pr_info("found shared page at phys addr 0x%llx:\n", >> - boot_params.tboot_addr); >> - pr_debug("version: %d\n", tboot->version); >> - pr_debug("log_addr: 0x%08x\n", tboot->log_addr); >> - pr_debug("shutdown_entry: 0x%x\n", tboot->shutdown_entry); >> - pr_debug("tboot_base: 0x%08x\n", tboot->tboot_base); >> - pr_debug("tboot_size: 0x%x\n", tboot->tboot_size); > > This is indeed rather ugly - and the other patch that removes a debug > check seems counterproductive as well. > > Do we know how many genuine bugs -Wstringop-overread-warning has > caught or is about to catch? > > I.e. the real workaround might be to turn off the -Wstringop-overread-warning, > until GCC-11 gets fixed? In GCC 10 -Wstringop-overread is a subset of -Wstringop-overflow. GCC 11 breaks it out as a separate warning to make it easier to control. Both warnings have caught some real bugs but they both have a nonzero rate of false positives. Other than bug reports we don't have enough data to say what their S/N ratio might be but my sense is that it's fairly high in general. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=wstringop-overread https://gcc.gnu.org/bugzilla/show_bug.cgi?id=wstringop-overflow In GCC 11, all access warnings expect objects to be either declared or allocated. Pointers with constant values are taken to point to nothing valid (as Arnd mentioned above, this is to detect invalid accesses to members of structs at address zero). One possible solution to the known address problem is to extend GCC attributes address and io that pin an object to a hardwired address to all targets (at the moment they're supported on just one or two targets). I'm not sure this can still happen before GCC 11 releases sometime in April or May. Until then, another workaround is to convert the fixed address to a volatile pointer before using it for the access, along the lines below. It should have only a negligible effect on efficiency. diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c index 4c09ba110204..76326b906010 100644 --- a/arch/x86/kernel/tboot.c +++ b/arch/x86/kernel/tboot.c @@ -67,7 +67,9 @@ void __init tboot_probe(void) /* Map and check for tboot UUID. */ set_fixmap(FIX_TBOOT_BASE, boot_params.tboot_addr); tboot = (struct tboot *)fix_to_virt(FIX_TBOOT_BASE); - if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) { + if (memcmp(&tboot_uuid, + (*(struct tboot* volatile *)(&tboot))->uuid, + sizeof(tboot->uuid))) { pr_warn("tboot at 0x%llx is invalid\n", boot_params.tboot_addr); tboot = NULL; return; Martin > > Thanks, > > Ingo >
On Mon, Mar 22, 2021 at 11:09 PM Martin Sebor <msebor@gmail.com> wrote: > On 3/22/21 2:29 PM, Ingo Molnar wrote: > > * Arnd Bergmann <arnd@kernel.org> wrote: > > > > I.e. the real workaround might be to turn off the -Wstringop-overread-warning, > > until GCC-11 gets fixed? > > In GCC 10 -Wstringop-overread is a subset of -Wstringop-overflow. > GCC 11 breaks it out as a separate warning to make it easier to > control. Both warnings have caught some real bugs but they both > have a nonzero rate of false positives. Other than bug reports > we don't have enough data to say what their S/N ratio might be > but my sense is that it's fairly high in general. > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=wstringop-overread > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=wstringop-overflow Unfortunately, stringop-overflow is one of the warnings that is completely disabled in the kernel at the moment, rather than enabled at one of the user-selectable higher warning levels. I have a patch series to change that and to pull some of these into the lower W=1 or W=2 levels or even enable them by default. To do this though, I first need to ensure that the actual output is empty for the normal builds. I added -Wstringop-overflow to the list of warnings I wanted to address because it is a new warning and only has a dozen or so occurrences throughout the kernel. > In GCC 11, all access warnings expect objects to be either declared > or allocated. Pointers with constant values are taken to point to > nothing valid (as Arnd mentioned above, this is to detect invalid > accesses to members of structs at address zero). > > One possible solution to the known address problem is to extend GCC > attributes address and io that pin an object to a hardwired address > to all targets (at the moment they're supported on just one or two > targets). I'm not sure this can still happen before GCC 11 releases > sometime in April or May. > > Until then, another workaround is to convert the fixed address to > a volatile pointer before using it for the access, along the lines > below. It should have only a negligible effect on efficiency. > > diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c > index 4c09ba110204..76326b906010 100644 > --- a/arch/x86/kernel/tboot.c > +++ b/arch/x86/kernel/tboot.c > @@ -67,7 +67,9 @@ void __init tboot_probe(void) > /* Map and check for tboot UUID. */ > set_fixmap(FIX_TBOOT_BASE, boot_params.tboot_addr); > tboot = (struct tboot *)fix_to_virt(FIX_TBOOT_BASE); > - if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) { > + if (memcmp(&tboot_uuid, > + (*(struct tboot* volatile *)(&tboot))->uuid, > + sizeof(tboot->uuid))) { > pr_warn("tboot at 0x%llx is invalid\n", I think a stray 'volatile' would raise too many red flags here, but I checked that the RELOC_HIDE() macro has the same effect, e.g. @@ -66,7 +67,8 @@ void __init tboot_probe(void) /* Map and check for tboot UUID. */ set_fixmap(FIX_TBOOT_BASE, boot_params.tboot_addr); - tboot = (struct tboot *)fix_to_virt(FIX_TBOOT_BASE); + /* RELOC_HIDE to prevent gcc warnings about NULL pointer */ + tboot = RELOC_HIDE(NULL, fix_to_virt(FIX_TBOOT_BASE)); if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) { pr_warn("tboot at 0x%llx is invalid\n", boot_params.tboot_addr); tboot = NULL; Arnd
* Martin Sebor <msebor@gmail.com> wrote: > > I.e. the real workaround might be to turn off the -Wstringop-overread-warning, > > until GCC-11 gets fixed? > > In GCC 10 -Wstringop-overread is a subset of -Wstringop-overflow. > GCC 11 breaks it out as a separate warning to make it easier to > control. Both warnings have caught some real bugs but they both > have a nonzero rate of false positives. Other than bug reports > we don't have enough data to say what their S/N ratio might be > but my sense is that it's fairly high in general. > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=wstringop-overread > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=wstringop-overflow > > In GCC 11, all access warnings expect objects to be either declared > or allocated. Pointers with constant values are taken to point to > nothing valid (as Arnd mentioned above, this is to detect invalid > accesses to members of structs at address zero). > > One possible solution to the known address problem is to extend GCC > attributes address and io that pin an object to a hardwired address > to all targets (at the moment they're supported on just one or two > targets). I'm not sure this can still happen before GCC 11 releases > sometime in April or May. > > Until then, another workaround is to convert the fixed address to > a volatile pointer before using it for the access, along the lines > below. It should have only a negligible effect on efficiency. Thank you for the detailed answer! I think I'll go with Arnd's original patch - which makes the code a slightly bit cleaner by separating out the check_tboot_version() check into a standalone function. The only ugly aspect is the global nature of the 'tboot' pointer - but that's a self-inflicted wound. I'd also guess that the S/N ratio somewhat unfairly penalizes this warning right now, because the kernel had a decade of growing real fixes via other efforts such as static and dynamic instrumentation as well. So the probability of false positive remaining is in fact higher, and going forward we should see a better S/N ratio of this warning. Most of which will never be seen by upstream maintainers, as the mishaps will stay at the individual developer level. :-) Thanks, Ingo
From: Martin Sebor > Sent: 22 March 2021 22:08 ... > In GCC 11, all access warnings expect objects to be either declared > or allocated. Pointers with constant values are taken to point to > nothing valid (as Arnd mentioned above, this is to detect invalid > accesses to members of structs at address zero). > > One possible solution to the known address problem is to extend GCC > attributes address and io that pin an object to a hardwired address > to all targets (at the moment they're supported on just one or two > targets). I'm not sure this can still happen before GCC 11 releases > sometime in April or May. A different solution is to define a normal C external data item and then assign a fixed address with an asm statement or in the linker script. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
From: David Laight > Sent: 24 March 2021 09:12 > > From: Martin Sebor > > Sent: 22 March 2021 22:08 > ... > > In GCC 11, all access warnings expect objects to be either declared > > or allocated. Pointers with constant values are taken to point to > > nothing valid (as Arnd mentioned above, this is to detect invalid > > accesses to members of structs at address zero). > > > > One possible solution to the known address problem is to extend GCC > > attributes address and io that pin an object to a hardwired address > > to all targets (at the moment they're supported on just one or two > > targets). I'm not sure this can still happen before GCC 11 releases > > sometime in April or May. > > A different solution is to define a normal C external data item > and then assign a fixed address with an asm statement or in > the linker script. Or stop gcc tracking the value by using: struct foo *foo = (void *)xxxxx; asm ("", "+r" (foo)); If the address is used more than once forcing it into a register is also likely to generate better code. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c index 4c09ba110204..f9af561c3cd4 100644 --- a/arch/x86/kernel/tboot.c +++ b/arch/x86/kernel/tboot.c @@ -49,6 +49,30 @@ bool tboot_enabled(void) return tboot != NULL; } +/* noinline to prevent gcc from warning about dereferencing constant fixaddr */ +static noinline __init bool check_tboot_version(void) +{ + if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) { + pr_warn("tboot at 0x%llx is invalid\n", boot_params.tboot_addr); + return false; + } + + if (tboot->version < 5) { + pr_warn("tboot version is invalid: %u\n", tboot->version); + return false; + } + + pr_info("found shared page at phys addr 0x%llx:\n", + boot_params.tboot_addr); + pr_debug("version: %d\n", tboot->version); + pr_debug("log_addr: 0x%08x\n", tboot->log_addr); + pr_debug("shutdown_entry: 0x%x\n", tboot->shutdown_entry); + pr_debug("tboot_base: 0x%08x\n", tboot->tboot_base); + pr_debug("tboot_size: 0x%x\n", tboot->tboot_size); + + return true; +} + void __init tboot_probe(void) { /* Look for valid page-aligned address for shared page. */ @@ -66,25 +90,9 @@ void __init tboot_probe(void) /* Map and check for tboot UUID. */ set_fixmap(FIX_TBOOT_BASE, boot_params.tboot_addr); - tboot = (struct tboot *)fix_to_virt(FIX_TBOOT_BASE); - if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) { - pr_warn("tboot at 0x%llx is invalid\n", boot_params.tboot_addr); + tboot = (void *)fix_to_virt(FIX_TBOOT_BASE); + if (!check_tboot_version()) tboot = NULL; - return; - } - if (tboot->version < 5) { - pr_warn("tboot version is invalid: %u\n", tboot->version); - tboot = NULL; - return; - } - - pr_info("found shared page at phys addr 0x%llx:\n", - boot_params.tboot_addr); - pr_debug("version: %d\n", tboot->version); - pr_debug("log_addr: 0x%08x\n", tboot->log_addr); - pr_debug("shutdown_entry: 0x%x\n", tboot->shutdown_entry); - pr_debug("tboot_base: 0x%08x\n", tboot->tboot_base); - pr_debug("tboot_size: 0x%x\n", tboot->tboot_size); } static pgd_t *tboot_pg_dir;