From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.2 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 87E6BC433C1 for ; Mon, 22 Mar 2021 22:08:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 684F0619AB for ; Mon, 22 Mar 2021 22:08:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230063AbhCVWIC (ORCPT ); Mon, 22 Mar 2021 18:08:02 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35378 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229730AbhCVWH4 (ORCPT ); Mon, 22 Mar 2021 18:07:56 -0400 Received: from mail-qt1-x82c.google.com (mail-qt1-x82c.google.com [IPv6:2607:f8b0:4864:20::82c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0F9B1C061574; Mon, 22 Mar 2021 15:07:56 -0700 (PDT) Received: by mail-qt1-x82c.google.com with SMTP id y2so11138275qtw.13; Mon, 22 Mar 2021 15:07:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=bp6cadiLp9JG4ot8VD8DrNUhLvtl3tOiutuJ7QXWznQ=; b=iaV83Ux9xBDgj/qom3Bk+Pnnd+Npq2g7l2Wcj4hhm0C9/Wqa5sWw8p1Jsa6IVtm0yS TdIiUfQGXTD6NiCLV6NBiRXyZP2uVw+Q2q6lU7LXwrWQJ/Maefvj0xV0Uec18FcSeRXF vIJU9u5du1ZOWFtkZrF5/qMHCMqhC93pFGIqrrKl6XUhkAUrPxcx4X0Batpu84izU0rl 6nUs9IEvPSvLgj3W8ej7UliIwlqDMHRm/QB8uQcgZsL5UpAnoTMEMr+Ed2fl6upuTDTk ++B/wzn6EYS99xzPoQfKZFL2om7c/ZVOPmZEz09Ms8KU+NvgtrohB8esdB1kDZFPOvri 10Xw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=bp6cadiLp9JG4ot8VD8DrNUhLvtl3tOiutuJ7QXWznQ=; b=jddVdzHLZz2NLlLHjp0TkxvOdobYmwIcsRfDz43S9CFMgwI6TEXRlThUYBHL7DFV9b gOa92HNWwoDSRkqVYVlI0BKYvme8oTkIYrywMDwuDEZD+f7Nd+SgW2vMFPU2Y/ZgHGv9 8DcdPpLuvX/4QwJl/eheuOGhYjNqzD2KIPddxqKjxkXpojo3sx2MHAgXKHfAG5vdvl8q gTh26lkjlS21BE/MxMOOpXmg3Fw12LsMjuu0FL9RGMZVLHpGJHK3OaCn/Oneo5TDlA81 SjFtjZKeXFt6gGKSdluw9RxLjVHv07zCPIOjWOd5byuVg2idPbZfrSZyASsQBeOVZ8wh qFNg== X-Gm-Message-State: AOAM531+E1ZFq+tytPnVB1oAQCFhOqNCBR3sw/ZixFYdAtr+N7KAvVtF mtiROIlC6hZ3TDVxxsn0+RE= X-Google-Smtp-Source: ABdhPJw2Q59a1ZLYugiGOh0k53LBnL3VNySQVKoaoySDrE/bFUhGmQDuP132X+ofXKkmUC96EG1Wbg== X-Received: by 2002:ac8:5281:: with SMTP id s1mr1870981qtn.293.1616450875002; Mon, 22 Mar 2021 15:07:55 -0700 (PDT) Received: from [192.168.0.41] (71-218-23-248.hlrn.qwest.net. [71.218.23.248]) by smtp.gmail.com with ESMTPSA id y19sm12052651qky.111.2021.03.22.15.07.51 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 22 Mar 2021 15:07:54 -0700 (PDT) Subject: Re: [PATCH 02/11] x86: tboot: avoid Wstringop-overread-warning To: Ingo Molnar , Arnd Bergmann Cc: linux-kernel@vger.kernel.org, Martin Sebor , Ning Sun , Thomas Gleixner , Ingo Molnar , Borislav Petkov , x86@kernel.org, Arnd Bergmann , Jani Nikula , Kalle Valo , Simon Kelley , James Smart , "James E.J. Bottomley" , Anders Larsen , Tejun Heo , Serge Hallyn , Imre Deak , 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" , Andrew Morton , Lu Baolu , Will Deacon References: <20210322160253.4032422-1-arnd@kernel.org> <20210322160253.4032422-3-arnd@kernel.org> <20210322202958.GA1955909@gmail.com> From: Martin Sebor Message-ID: Date: Mon, 22 Mar 2021 16:07:50 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <20210322202958.GA1955909@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: On 3/22/21 2:29 PM, Ingo Molnar wrote: > > * Arnd Bergmann wrote: > >> From: Arnd Bergmann >> >> 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 >> --- >> 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 >