linux-kernel - Re: [GIT PULL] x86/shstk for 6.4

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-=wiuVXTfgapmjYQvrEDzn3naF2oYnHuky+feEJSj_G_yFQ@mail.gmail.com>
Date:   Sat, 6 May 2023 12:34:53 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>
Subject: Re: [GIT PULL] x86/shstk for 6.4

On Fri, Apr 28, 2023 at 5:40 PM Dave Hansen <dave.hansen@...el.com> wrote:
>
> I'd be happy to prepare another pull request for the pile up to there if
> it sounds sane to everyone else.

So I'm going through the original pull request now - I was really
hoping to have been able to do that earlier, but there kept being all
these small pending other issues.

And I'm about a quarter in, haven't even gotten to the meat yet, and
I've already found a bug.

Commit 74fd30bd28e4 ("mm: Make pte_mkwrite() take a VMA") seems to be
just completely broken at least on arm 3-level page tables:

Spot the problem when I distill it down to just a few lines):

    -PMD_BIT_FUNC(mkwrite,   &= ~L_PMD_SECT_RDONLY);
    +static inline pmd_t pmd_mkwrite(pmd_t pmd, struct vm_area_struct *vma)
    +{
    +       pmd_val(pmd) |= L_PMD_SECT_RDONLY;
    +       return pmd;
    +}

and I think this whole area was done entirely incorrectly - and that
incorrect approach is then why this bug happened.

I think the first patch should have just have completely mindlessly
renamed every architecture "pmd/pte_mkwrite()" function as
"pmd/pte_mkwrite_kernel()", and added a single

    #ifndef pte_mkwrite
    static inline pte_t pte_mkwrite(pte_t pte)
    { return pte_mkwrite_kernel(pte); }
    #endif

in <linux/pgtable.h>.

IOW, it should make it *completely* obvious that absolutely no
semantic changes happened, and all that happened that it added that
single wrapper layer. And all those architecture changes would be
trivial one-liners.

The only possibly subtle thing would be that some existing

   #include <asm/pgtable.h>

include might need to be changed to

   #include <linux/pgtable.h>

instead, because we do have a number of users that seem to include
just the bare asm version.

>From some quick grepping, I don't think any of them use 'pte_dirty()'
or 'pmd_dirty()', but it needs *checking*.

Then, the "add vma" thing would change the above wrapper to literally
just have the 'vma', but to not use it, and still just call
'pte_mkwrite_kernel(pte)'.

Again, absolutely obvious that there are zero semantic changes to any
architectures that don't care.

And finally - for the only architecture that *does* care, ie x86, do
implement the new pte_mkwrite(vma,pte), and do a

  #define pte_mkwrite pte_mkwrite

to let the generic header know that there's now an
architecture-specific version for it, and it shouldn't do that wrapper
that just falls back on the "kernel" version.

End result: all those architectures that do *not* want the vma
argument don't need to do any extra work, and they just implement the
old version, and the only thing that happened was that it was renamed.

Because I really don't want to pull this series as-is, when I found
what looks like a "this broke an architecture that DOES NOT EVEN CARE"
bug in the series.

And yes, my bad for not getting to this earlier to notice this.

Or alternatively - your bad for not going through this with a fine
comb like I started doing.

             Linus