Linux-SCSI Archive on lore.kernel.org
 help / color / Atom feed
From: Martin Sebor <msebor@gmail.com>
To: Ingo Molnar <mingo@kernel.org>, Arnd Bergmann <arnd@kernel.org>
Cc: linux-kernel@vger.kernel.org, Martin Sebor <msebor@gcc.gnu.org>,
	Ning Sun <ning.sun@intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	x86@kernel.org, Arnd Bergmann <arnd@arndb.de>,
	Jani Nikula <jani.nikula@linux.intel.com>,
	Kalle Valo <kvalo@codeaurora.org>,
	Simon Kelley <simon@thekelleys.org.uk>,
	James Smart <james.smart@broadcom.com>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	Anders Larsen <al@alarsen.net>, Tejun Heo <tj@kernel.org>,
	Serge Hallyn <serge@hallyn.com>, Imre Deak <imre.deak@intel.com>,
	linux-arm-kernel@lists.infradead.org,
	tboot-devel@lists.sourceforge.net,
	intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	ath11k@lists.infradead.org, linux-wireless@vger.kernel.org,
	netdev@vger.kernel.org, linux-scsi@vger.kernel.org,
	cgroups@vger.kernel.org, linux-security-module@vger.kernel.org,
	"H. Peter Anvin" <hpa@zytor.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Lu Baolu <baolu.lu@linux.intel.com>,
	Will Deacon <will@kernel.org>
Subject: Re: [PATCH 02/11] x86: tboot: avoid Wstringop-overread-warning
Date: Mon, 22 Mar 2021 16:07:50 -0600
Message-ID: <b944a853-0e4b-b767-0175-cc2c1edba759@gmail.com> (raw)
In-Reply-To: <20210322202958.GA1955909@gmail.com>

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
> 


  parent reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-22 16:02 [PATCH 00/11] treewide: address gcc-11 -Wstringop-overread warnings Arnd Bergmann
2021-03-22 16:02 ` [PATCH 01/11] x86: compressed: avoid gcc-11 -Wstringop-overread warning Arnd Bergmann
2021-03-22 16:02 ` [PATCH 02/11] x86: tboot: avoid Wstringop-overread-warning Arnd Bergmann
2021-03-22 20:29   ` Ingo Molnar
2021-03-22 21:39     ` Arnd Bergmann
2021-03-22 22:07     ` Martin Sebor [this message]
2021-03-22 22:49       ` Arnd Bergmann
2021-03-22 23:13       ` Ingo Molnar
2021-03-24  9:11       ` David Laight
2021-03-24 10:39         ` David Laight
2021-03-22 16:02 ` [PATCH 03/11] security: commoncap: fix -Wstringop-overread warning Arnd Bergmann
2021-03-22 16:31   ` Christian Brauner
2021-03-24 20:50   ` James Morris
2021-03-22 16:02 ` [PATCH 04/11] ath11: Wstringop-overread warning Arnd Bergmann
2021-03-22 16:02 ` [PATCH 05/11] qnx: avoid -Wstringop-overread warning Arnd Bergmann
2021-03-22 16:02 ` [PATCH 06/11] cgroup: fix -Wzero-length-bounds warnings Arnd Bergmann
2021-03-30  8:41   ` Michal Koutný
2021-03-30  9:00     ` Arnd Bergmann
2021-03-30 14:44       ` Michal Koutný
2021-03-22 16:02 ` [PATCH 07/11] ARM: sharpsl_param: work around -Wstringop-overread warning Arnd Bergmann
2021-03-22 16:02 ` [PATCH 08/11] atmel: avoid gcc -Wstringop-overflow warning Arnd Bergmann
2021-03-22 16:02 ` [PATCH 09/11] scsi: lpfc: fix gcc -Wstringop-overread warning Arnd Bergmann
2021-03-22 16:02 ` [PATCH 10/11] drm/i915: avoid stringop-overread warning on pri_latency Arnd Bergmann
2021-03-24 15:30   ` Jani Nikula
2021-03-24 17:22     ` Ville Syrjälä
2021-03-22 16:02 ` [PATCH 11/11] [RFC] drm/i915/dp: fix array overflow warning Arnd Bergmann
2021-03-25  8:05   ` Jani Nikula
2021-03-25  9:53     ` Arnd Bergmann
2021-03-25 14:49       ` Martin Sebor
2021-03-30 10:56   ` Hans de Goede
2021-04-06  4:53 ` [PATCH 00/11] treewide: address gcc-11 -Wstringop-overread warnings Martin K. Petersen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b944a853-0e4b-b767-0175-cc2c1edba759@gmail.com \
    --to=msebor@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=al@alarsen.net \
    --cc=arnd@arndb.de \
    --cc=arnd@kernel.org \
    --cc=ath11k@lists.infradead.org \
    --cc=baolu.lu@linux.intel.com \
    --cc=bp@alien8.de \
    --cc=cgroups@vger.kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=hpa@zytor.com \
    --cc=imre.deak@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=james.smart@broadcom.com \
    --cc=jani.nikula@linux.intel.com \
    --cc=jejb@linux.ibm.com \
    --cc=kvalo@codeaurora.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=mingo@redhat.com \
    --cc=msebor@gcc.gnu.org \
    --cc=netdev@vger.kernel.org \
    --cc=ning.sun@intel.com \
    --cc=serge@hallyn.com \
    --cc=simon@thekelleys.org.uk \
    --cc=tboot-devel@lists.sourceforge.net \
    --cc=tglx@linutronix.de \
    --cc=tj@kernel.org \
    --cc=will@kernel.org \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-SCSI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-scsi/0 linux-scsi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-scsi linux-scsi/ https://lore.kernel.org/linux-scsi \
		linux-scsi@vger.kernel.org
	public-inbox-index linux-scsi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-scsi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git