linux-kernel - Re: [GIT pull] x86/pti for 5.4-rc1

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=whbOhAmc3FEhnEkdM9AXFuKM+964r+uzzm_Q9qFaxTC7g@mail.gmail.com>
Date:   Tue, 17 Sep 2019 12:01:53 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Song Liu <songliubraving@...com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        "the arch/x86 maintainers" <x86@...nel.org>
Subject: Re: [GIT pull] x86/pti for 5.4-rc1

On Tue, Sep 17, 2019 at 11:49 AM Song Liu <songliubraving@...com> wrote:
>
> I guess we need something like the following?
>
> diff --git i/arch/x86/mm/pti.c w/arch/x86/mm/pti.c
> index b196524759ec..7846916c3bcd 100644
> --- i/arch/x86/mm/pti.c
> +++ w/arch/x86/mm/pti.c
> @@ -306,6 +306,8 @@ pti_clone_pgtable(unsigned long start, unsigned long end,
>  {
>         unsigned long addr;
>
> +       if (WARN_ON_ONCE(start & ~PAGE_MASK))
> +               return;

I don't think we ever care about the low bits of the address below the
page mask, so this one probably wouldn't make any difference.

To match the other cases, I'd make it just a plain

        WARN_ON_ONCE(start & ~PAGE_MASK));

and leave it at that. The existing commit added the warning, but then
just made the code work despite it all.  I'd continue that same logic.

>                 if (pmd_large(*pmd) || level == PTI_CLONE_PMD) {
> +                       if (WARN_ON_ONCE(addr & ~PMD_MASK))
> +                               return;
>                         target_pmd = pti_user_pagetable_walk_pmd(addr);
>                         if (WARN_ON(!target_pmd))
>                                 return;

Again, I think to match the other cases, I'd just do

-                       addr += PMD_SIZE;
+                       WARN_ON_ONCE(addr & ~PMD_MASK);
+                       addr = round_up(addr + 1, PMD_SIZE);

which now admittedly clones too much, but _does_ clone the requested range.

But maybe it really doesn't matter, since this condition just
shouldn't happen in the first place.

And arguably, the "clone more than requested" issue is true, and maybe
your "warn and refuse to clone by returning" is the right thing to do.

So I have very few strong opinions in this area, I just reacted to
looking at the patch that it didn't seem to cover all the cases.

> > Also, it would have been lovely to have some background on how this
> > was even noticed. The link in the commit message goes to the
> > development thread, but that one doesn't have the original report from
> > Song either.
>
> I didn't really notice any issue. I was debugging an increase in iTLB
> miss rate, which was caused by splitting of kernel text page table,
> and was fixed by Thomas in:
>
> https://lore.kernel.org/lkml/alpine.DEB.2.21.1908282355340.1938@nanos.tec.linutronix.de/
>
> I mistakenly suspected the issue was caused by the pti code, and
> mistakenly proposed the first patch here. It turned out to be useful,
> but it is not related to the original issue.

Ok, thanks for the explanation.

              Linus