linux-kernel - Re: [PATCH 1/1] x86/mm: Use proper mask when setting PUD mapping

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-=wieSGev8B_Xq3R55gkNLU=cuo+idJtMJYqQ+EAJ4HeJgA@mail.gmail.com>
Date:   Thu, 18 Aug 2022 20:08:08 -0700
From:   Linus Torvalds <torvalds@...uxfoundation.org>
To:     Aaron Lu <aaron.lu@...el.com>
Cc:     security@...nel.org, lkml <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Michal Hocko <mhocko@...e.com>,
        Logan Gunthorpe <logang@...tatee.com>
Subject: Re: [PATCH 1/1] x86/mm: Use proper mask when setting PUD mapping

On Thu, Aug 18, 2022 at 7:30 PM Aaron Lu <aaron.lu@...el.com> wrote:
>
> -                       prot = __pgprot(pgprot_val(prot) | __PAGE_KERNEL_LARGE);
> +                       prot = __pgprot_mask(pgprot_val(prot) | __PAGE_KERNEL_LARGE);

The patch looks "ObviouslyCorrect(tm)" to me, but I have to admit that
I absolutely hate how we use the pte helpers for all the levels.

It gets even worse when we do that

                        set_pte_init((pte_t *)pud,
                                     pfn_pte((paddr & PUD_MASK) >> PAGE_SHIFT,
                                             prot),
                                     init);

on the next lines, and I don't understand why this doesn't use
"set_pud_init()" here.

It's probably something obvious, like "using set_pud_init() would mean
that we'd have to cast the *second* argument instead, because we don't
have a pfd_pud() function".

But it all makes me go a bit eww, and also makes me suspect I am
missing something else too, and that my "this looks
ObviouslyCorrect(tm)" is thus worthless.

Also, I don't understand why we use that __PAGE_KERNEL_LARGE at all.
We already have a valid set of protection bits, that had gotten
properly masked previously.

Isn't the only bit we actually want to set "_PAGE_PSE"?

IOW, I get the feeling that that patch should instead just be

-                       prot = __pgprot(pgprot_val(prot) | __PAGE_KERNEL_LARGE);
+                       prot = __pgprot(pgprot_val(prot) | _PAGE_PSE);

and we should never need to mask anything off at all with
__pgprot_mask() - the bug was really that we set way too many bits.

But again, I *also* have the feeling that I'm missing something important.

Ingo, Thomas, any others who know this code better than me by now - comments?

                Linus