From: "Jason A. Donenfeld" <Jason@zx2c4.com>
To: Joerg Roedel <joro@8bytes.org>
Cc: X86 ML <x86@kernel.org>,
Dave Hansen <dave.hansen@linux.intel.com>,
Andy Lutomirski <luto@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
"H . Peter Anvin" <hpa@zytor.com>,
Mike Rapoport <rppt@linux.ibm.com>,
Joerg Roedel <jroedel@suse.de>,
Linus Torvalds <torvalds@linux-foundation.org>,
Andrew Morton <akpm@linux-foundation.org>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] x86/mm/64: Do not dereference non-present PGD entries
Date: Fri, 7 Aug 2020 11:52:09 +0200 [thread overview]
Message-ID: <CAHmME9rPfsMJ_btZecWrJN=R6FqAFmjDzCC_tMGk01+R6gJAPw@mail.gmail.com> (raw)
In-Reply-To: <20200807084013.7090-1-joro@8bytes.org>
On Fri, Aug 7, 2020 at 10:40 AM Joerg Roedel <joro@8bytes.org> wrote:
>
> From: Joerg Roedel <jroedel@suse.de>
>
> The code for preallocate_vmalloc_pages() was written under the
> assumption that the p4d_offset() and pud_offset() functions will perform
> present checks before dereferencing the parent entries.
>
> This assumption is wrong an leads to a bug in the code which causes the
> physical address found in the PGD be used as a page-table page, even if
> the PGD is not present.
>
> So the code flow currently is:
>
> pgd = pgd_offset_k(addr);
> p4d = p4d_offset(pgd, addr);
> if (p4d_none(*p4d))
> p4d = p4d_alloc(&init_mm, pgd, addr);
>
> This lacks a check for pgd_none() at least, the correct flow would be:
>
> pgd = pgd_offset_k(addr);
> if (pgd_none(*pgd))
> p4d = p4d_alloc(&init_mm, pgd, addr);
> else
> p4d = p4d_offset(pgd, addr);
>
> But this is the same flow that the p4d_alloc() and the pud_alloc()
> functions use internally, so there is no need to duplicate them.
>
> Remove the p?d_none() checks from the function and just call into
> p4d_alloc() and pud_alloc() to correctly pre-allocate the PGD entries.
>
> Reported-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Fixes: 6eb82f994026 ("x86/mm: Pre-allocate P4D/PUD pages for vmalloc area")
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
> arch/x86/mm/init_64.c | 31 +++++++++++++------------------
> 1 file changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index 3f4e29a78f2b..449e071240e1 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -1253,28 +1253,23 @@ static void __init preallocate_vmalloc_pages(void)
> p4d_t *p4d;
> pud_t *pud;
>
> - p4d = p4d_offset(pgd, addr);
> - if (p4d_none(*p4d)) {
> - /* Can only happen with 5-level paging */
> - p4d = p4d_alloc(&init_mm, pgd, addr);
> - if (!p4d) {
> - lvl = "p4d";
> - goto failed;
> - }
> - }
> + lvl = "p4d";
> + p4d = p4d_alloc(&init_mm, pgd, addr);
> + if (!p4d)
> + goto failed;
>
> + /*
> + * With 5-level paging the P4D level is not folded. So the PGDs
> + * are now populated and there is no need to walk down to the
> + * PUD level.
> + */
> if (pgtable_l5_enabled())
> continue;
>
> - pud = pud_offset(p4d, addr);
> - if (pud_none(*pud)) {
> - /* Ends up here only with 4-level paging */
> - pud = pud_alloc(&init_mm, p4d, addr);
> - if (!pud) {
> - lvl = "pud";
> - goto failed;
> - }
> - }
> + lvl = "pud";
> + pud = pud_alloc(&init_mm, p4d, addr);
> + if (!pud)
> + goto failed;
> }
>
> return;
> --
> 2.26.2
This appears to fix the issue, so:
Tested-by: Jason A. Donenfeld <Jason@zx2c4.com>
next prev parent reply other threads:[~2020-08-07 9:52 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-07 8:40 [PATCH] x86/mm/64: Do not dereference non-present PGD entries Joerg Roedel
2020-08-07 9:52 ` Jason A. Donenfeld [this message]
2020-08-07 10:07 ` Mike Rapoport
2020-08-10 14:27 ` Dave Hansen
2020-08-10 15:53 ` Mike Rapoport
2020-08-13 19:21 ` Ingo Molnar
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='CAHmME9rPfsMJ_btZecWrJN=R6FqAFmjDzCC_tMGk01+R6gJAPw@mail.gmail.com' \
--to=jason@zx2c4.com \
--cc=akpm@linux-foundation.org \
--cc=dave.hansen@linux.intel.com \
--cc=hpa@zytor.com \
--cc=joro@8bytes.org \
--cc=jroedel@suse.de \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=peterz@infradead.org \
--cc=rppt@linux.ibm.com \
--cc=torvalds@linux-foundation.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).