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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20210322202958.GA1955909@gmail.com>
Date:   Mon, 22 Mar 2021 21:29:58 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Arnd Bergmann <arnd@...nel.org>
Cc:     linux-kernel@...r.kernel.org, Martin Sebor <msebor@....gnu.org>,
        Ning Sun <ning.sun@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org, Arnd Bergmann <arnd@...db.de>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Kalle Valo <kvalo@...eaurora.org>,
        Simon Kelley <simon@...kelleys.org.uk>,
        James Smart <james.smart@...adcom.com>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        Anders Larsen <al@...rsen.net>, Tejun Heo <tj@...nel.org>,
        Serge Hallyn <serge@...lyn.com>,
        Imre Deak <imre.deak@...el.com>,
        linux-arm-kernel@...ts.infradead.org,
        tboot-devel@...ts.sourceforge.net, intel-gfx@...ts.freedesktop.org,
        dri-devel@...ts.freedesktop.org, ath11k@...ts.infradead.org,
        linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
        linux-scsi@...r.kernel.org, cgroups@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        "H. Peter Anvin" <hpa@...or.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Lu Baolu <baolu.lu@...ux.intel.com>,
        Will Deacon <will@...nel.org>
Subject: Re: [PATCH 02/11] x86: tboot: avoid Wstringop-overread-warning


* Arnd Bergmann <arnd@...nel.org> wrote:

> From: Arnd Bergmann <arnd@...db.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@...db.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