| Message ID | Yytb67xvrnctxnEe@work (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [next] mm/pgtable: Fix multiple -Wstringop-overflow warnings | expand |
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@kernel.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@chromium.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
On Wed, 21 Sep 2022 13:46:03 -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. > > [...] Applied to for-next/hardening with tweaks to the commit log, thanks! [1/1] mm/pgtable: Fix multiple -Wstringop-overflow warnings https://git.kernel.org/kees/c/9af98f78d2cd
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; if (paravirt_pgd_alloc(mm) != 0) @@ -451,17 +453,22 @@ pgd_t *pgd_alloc(struct mm_struct *mm) spin_lock(&pgd_lock); pgd_ctor(mm, pgd); - pgd_prepopulate_pmd(mm, pgd, pmds); - pgd_prepopulate_user_pmd(mm, pgd, u_pmds); + if (sizeof(pmds) != 0) + pgd_prepopulate_pmd(mm, pgd, pmds); + + if (sizeof(u_pmds) != 0) + pgd_prepopulate_user_pmd(mm, pgd, u_pmds); spin_unlock(&pgd_lock); return pgd; out_free_user_pmds: - free_pmds(mm, u_pmds, PREALLOCATED_USER_PMDS); + if (sizeof(u_pmds) != 0) + free_pmds(mm, u_pmds, PREALLOCATED_USER_PMDS); out_free_pmds: - free_pmds(mm, pmds, PREALLOCATED_PMDS); + if (sizeof(pmds) != 0) + free_pmds(mm, pmds, PREALLOCATED_PMDS); out_free_pgd: _pgd_free(pgd); out:
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. 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: 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@kernel.org> --- arch/x86/mm/pgtable.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)