Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <Yv71VPSjyy26v/Xu@ziqianlu-desk1> <Yv71qbuHkSsLGhvJ@ziqianlu-desk1>
In-Reply-To: <Yv71qbuHkSsLGhvJ@ziqianlu-desk1>
From:   Linus Torvalds <torvalds@linuxfoundation.org>
Date:   Thu, 18 Aug 2022 20:08:08 -0700
Message-ID: <CAHk-=wieSGev8B_Xq3R55gkNLU=cuo+idJtMJYqQ+EAJ4HeJgA@mail.gmail.com>
Subject: Re: [PATCH 1/1] x86/mm: Use proper mask when setting PUD mapping
To:     Aaron Lu <aaron.lu@intel.com>
Cc:     security@kernel.org, lkml <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Andy Lutomirski <luto@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Michal Hocko <mhocko@suse.com>,
        Logan Gunthorpe <logang@deltatee.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Aug 18, 2022 at 7:30 PM Aaron Lu <aaron.lu@intel.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