Re: [PATCH 02/11] x86: tboot: avoid Wstringop-overread-warning

[Arnd Bergmann]

On Mon, Mar 22, 2021 at 11:09 PM Martin Sebor <msebor@xxxxxxxxx> wrote:
> On 3/22/21 2:29 PM, Ingo Molnar wrote:
> > * Arnd Bergmann <arnd@xxxxxxxxxx> 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