Date:   Wed, 5 Oct 2022 14:40:52 -0700
From:   Kees Cook <keescook@...omium.org>
To:     "Gustavo A. R. Silva" <gustavoars@...nel.org>
Cc:     Dave Hansen <dave.hansen@...ux.intel.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] mm/pgtable: Fix multiple -Wstringop-overflow
 warnings

On Wed, Sep 21, 2022 at 01:46:03PM -0500, Gustavo A. R. Silva wrote:
> The actual size of the following arrays at run-time depends on
> CONFIG_X86_PAE.
> 
> 427         pmd_t *u_pmds[MAX_PREALLOCATED_USER_PMDS];
> 428         pmd_t *pmds[MAX_PREALLOCATED_PMDS];
> 
> If CONFIG_X86_PAE is not enabled, their final size will be zero. In that
> case, the compiler complains about trying to access objects of size zero
> when calling functions where these objects are passed as arguments.

The main point is that a zero-length array isn't considered to have legal
storage by C -- strictly speaking, a compiler isn't wrong to complain
about this (and it has in the past too, for this code):

static void pgd_prepopulate_pmd(struct mm_struct *mm, pgd_t *pgd, pmd_t *pmds[])
{
        p4d_t *p4d;
        pud_t *pud;
        int i;

        if (PREALLOCATED_PMDS == 0) /* Work around gcc-3.4.x bug */
                return;
...

But gcc has gotten smart enough to say "you can't pass this as an array
because it doesn't actually exist". No amount of inlining, etc, helps
because it's strictly looking at the function arguments and the local
storage declaration.

Note that this is basically one of the last warnings generated by
-Wstringop-overflow -- many of the other warnings that were fixed as
part of getting this option enabled have been real bugs. We want this
warning enabled, even if it's a bit more strict than some code is happy
about. :)

> Fix this by sanity-checking the size of those arrays just before the
> function calls. Also, the following warnings are fixed by these changes
> when building with GCC-11 and -Wstringop-overflow enabled:

And with GCC-12 too.

> arch/x86/mm/pgtable.c:437:13: warning: ‘preallocate_pmds.constprop’ accessing 8 bytes in a region of size 0 [-Wstringop-overflow=]
> arch/x86/mm/pgtable.c:440:13: warning: ‘preallocate_pmds.constprop’ accessing 8 bytes in a region of size 0 [-Wstringop-overflow=]
> arch/x86/mm/pgtable.c:462:9: warning: ‘free_pmds.constprop’ accessing 8 bytes in a region of size 0 [-Wstringop-overflow=]
> arch/x86/mm/pgtable.c:455:9: warning: ‘pgd_prepopulate_user_pmd’ accessing 8 bytes in a region of size 0 [-Wstringop-overflow=]
> arch/x86/mm/pgtable.c:464:9: warning: ‘free_pmds.constprop’ accessing 8 bytes in a region of size 0 [-Wstringop-overflow=]
> 
> This helps with the ongoing efforts to globally enable
> -Wstringop-overflow.
> 
> Link: https://github.com/KSPP/linux/issues/203
> Link: https://github.com/KSPP/linux/issues/181
> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>

BTW, this should be considered "v3", and include links to the history:

v2: https://lore.kernel.org/lkml/20220509194541.GA91598@embeddedor/
v1: https://lore.kernel.org/lkml/20220401005834.GA182932@embeddedor/

> ---
>  arch/x86/mm/pgtable.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> index 8525f2876fb4..5116df6a308c 100644
> --- a/arch/x86/mm/pgtable.c
> +++ b/arch/x86/mm/pgtable.c
> @@ -434,10 +434,12 @@ pgd_t *pgd_alloc(struct mm_struct *mm)
>  
>  	mm->pgd = pgd;
>  
> -	if (preallocate_pmds(mm, pmds, PREALLOCATED_PMDS) != 0)
> +	if (sizeof(pmds) != 0 &&
> +			preallocate_pmds(mm, pmds, PREALLOCATED_PMDS) != 0)
>  		goto out_free_pgd;
>  
> -	if (preallocate_pmds(mm, u_pmds, PREALLOCATED_USER_PMDS) != 0)
> +	if (sizeof(u_pmds) != 0 &&
> +			preallocate_pmds(mm, u_pmds, PREALLOCATED_USER_PMDS) != 0)
>  		goto out_free_pmds;

The alternative to this is to make the originally suggested change
from v1: change the pmds argument from an array pointer to a pointer
pointer. That situation is considered "legal" for C in the sense that
it does not have a way to reason about the storage. i.e.:

-static void pgd_prepopulate_pmd(struct mm_struct *mm, pgd_t *pgd, pmd_t *pmds[])
+static void pgd_prepopulate_pmd(struct mm_struct *mm, pgd_t *pgd, pmd_t **pmds)

With the above change, there's no difference in binary output, and the
compiler warning is silenced.

However, with this email's patch, the compiler can actually figure out
that it isn't using the code at all, and it gets dropped:

   text    data     bss     dec     hex filename
   8218     718      32    8968    2308 arch/x86/mm/pgtable.o.before
   7765     694      32    8491    212b arch/x86/mm/pgtable.o.after

Given that result (fixing a warning and reducing image size), this look
like a distinct win.

Reviewed-by: Kees Cook <keescook@...omium.org>

Note, we can add this change:

diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
index a932d7712d85..b311e4aa42fc 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -299,9 +299,6 @@ static void pgd_prepopulate_pmd(struct mm_struct *mm, pgd_t *pgd, pmd_t *pmds[])
 	pud_t *pud;
 	int i;
 
-	if (PREALLOCATED_PMDS == 0) /* Work around gcc-3.4.x bug */
-		return;
-
 	p4d = p4d_offset(pgd, 0);
 	pud = pud_offset(p4d, 0);
 

Which would get us to even more lines removed:

  1 file changed, 13 insertions(+), 9 deletions(-)

:)

-Kees

-- 
Kees Cook

Powered by blists - more mailing lists