Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 15.0 \(3693.60.0.1.1\))
Subject: Re: [RESEND PATCH v3 2/5] x86/mm: check exec permissions on fault
From:   Nadav Amit <nadav.amit@gmail.com>
In-Reply-To: <70e08bd5-187a-daee-2822-1d9a437a9cff@intel.com>
Date:   Fri, 11 Mar 2022 13:16:59 -0800
Cc:     Linux-MM <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Andrew Cooper <andrew.cooper3@citrix.com>,
        Andy Lutomirski <luto@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Peter Xu <peterx@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Will Deacon <will@kernel.org>, Yu Zhao <yuzhao@google.com>,
        Nick Piggin <npiggin@gmail.com>,
        "x86@kernel.org" <x86@kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <E46E0DA7-6427-4733-9D43-6CC1107194D6@gmail.com>
References: <20220311190749.338281-1-namit@vmware.com>
 <20220311190749.338281-3-namit@vmware.com>
 <a2a43395-b848-a4f9-4065-109387680701@intel.com>
 <AC8D21EA-CD32-4F9F-B5C1-ED8804EC76FF@vmware.com>
 <70e08bd5-187a-daee-2822-1d9a437a9cff@intel.com>
To:     Dave Hansen <dave.hansen@intel.com>
Precedence: bulk


> On Mar 11, 2022, at 12:59 PM, Dave Hansen <dave.hansen@intel.com> =
wrote:
>=20
> On 3/11/22 12:38, Nadav Amit wrote:
>>> On Mar 11, 2022, at 11:41 AM, Dave Hansen <dave.hansen@intel.com> =
wrote:
> ...
>>> Can any sane code trigger this?
>>=20
>> Well, regarding this question and the previous one: I do not think =
that
>> this scenario is possible today since mprotect() holds the mmap_lock
>> for write. There is no other code that I am aware of that toggles
>> the NX bit on a present entry.
>>=20
>> But I will not bet my life on it. That=E2=80=99s the reason for the =
somewhat
>> vague phrasing that I used.
>=20
> =46rom the userspace perspective, mmap(MAP_FIXED) can do this too.  =
But,
> sane userspace can't rely on the syscall to have done any work and the
> TLB flushing is currently done before the syscall returns.
>=20
> I'd put it this way:
>=20
> 	Today, it is possible for a thread to end up in access_error()
> 	for a PF_INSN fault and observe a VM_EXEC VMA.  If you are
> 	generous, this could be considered a spurious fault.
>=20
> 	However, the faulting thread would have had to race with the
> 	thread which was changing the PTE and the VMA and is currently
> 	*in* mprotect() (or some other syscall).  In other words, the
> 	faulting thread can encounter this situation, but it never had
> 	any assurance from the kernel that it wouldn't fault.  This is
> 	because the faulting thread never had a chance to observe the
> 	syscall return.
>=20
> 	There is no evidence that the existing behavior can cause any
> 	issues with sane userspace.

Done. Thanks.

>=20
>>>> index d0074c6ed31a..ad0ef0a6087a 100644
>>>> --- a/arch/x86/mm/fault.c
>>>> +++ b/arch/x86/mm/fault.c
>>>> @@ -1107,10 +1107,28 @@ access_error(unsigned long error_code, =
struct vm_area_struct *vma)
>>>> 				       (error_code & X86_PF_INSTR), =
foreign))
>>>> 		return 1;
>>>>=20
>>>> -	if (error_code & X86_PF_WRITE) {
>>>> +	if (error_code & (X86_PF_WRITE | X86_PF_INSTR)) {
>>>> +		/*
>>>> +		 * CPUs are not expected to set the two error code bits
>>>> +		 * together, but to ensure that hypervisors do not =
misbehave,
>>>> +		 * run an additional sanity check.
>>>> +		 */
>>>> +		if ((error_code & (X86_PF_WRITE|X86_PF_INSTR)) =3D=3D
>>>> +					(X86_PF_WRITE|X86_PF_INSTR)) {
>>>> +			WARN_ON_ONCE(1);
>>>> +			return 1;
>>>> +		}
>>>=20
>>> access_error() is only used on the do_user_addr_fault() side of =
things.
>>> Can we stick this check somewhere that also works for kernel address
>>> faults?  This is a generic sanity check.  It can also be in a =
separate
>>> patch.
>>=20
>> I can wrap it in a different function and also call it from
>> do_kern_addr_fault() or spurious_kernel_fault().
>>=20
>> Anyhow, spurious_kernel_fault() should handle spurious faults on
>> executable code correctly.=20
>=20
> This is really about checking the sanity of the "hardware"-provided
> error code.  Let's just do it in  handle_page_fault(), maybe hidden in =
a
> function like:
>=20
> void check_error_code_sanity(unsigned long error_code)
> {
> 	WARN_ON_ONCE(...);
> }
>=20
> You can leave the X86_PF_PK check in place for now.  It's probably =
going
> away soon anyway.

Done. Thanks. But note that removing the check from access_error() means
that if the assertion is broken, userspace might crash inadvertently
(in contrast to the version I sent, which would have potentially led to
infinite stream of page-faults). I don=E2=80=99t know which behavior is =
better,
so let=E2=80=99s go with your version and just hope it doesn=E2=80=99t =
happen.

>=20
>>> Also, we should *probably* stop talking about CPUs here.  If there's
>>> ever something wonky with error code bits, I'd put my money on a =
weird
>>> hypervisor before any kind of CPU issue.
>>=20
>> I thought I manage to convey exactly that in the comment. Can you =
provide
>> a better phrasing?
>=20
> Maybe:
>=20
> 	/*
> 	 * X86_PF_INSTR for instruction _fetches_.  Fetches never write.
> 	 * X86_PF_WRITE should never be set with X86_PF_INSTR.
> 	 *
> 	 * This is most likely due to a buggy hypervisor.
> 	 */

Done, thank you.