Re: [RESEND PATCH v3 2/5] x86/mm: check exec permissions on fault

[Nadav Amit <namit@vmware.com>]

> On Mar 11, 2022, at 11:41 AM, Dave Hansen <dave.hansen@intel.com> wrote:
> 
> On 3/11/22 11:07, Nadav Amit wrote:
>> From: Nadav Amit <namit@vmware.com>
>> 
>> access_error() currently does not check for execution permission
>> violation. As a result, spurious page-faults due to execution permission
>> violation cause SIGSEGV.
> 
> This is a bit muddy on the problem statement.  I get that spurious
> faults can theoretically cause this, but *do* they in practice on
> current kernels?
> 
>> It appears not to be an issue so far, but the next patches avoid TLB
>> flushes on permission promotion, which can lead to this scenario. nodejs
>> for instance crashes when TLB flush is avoided on permission promotion.
> 
> By "it appears not to be an issue", do you mean that this suboptimal
> behavior can not be triggered, period?  Or, it can be triggered but
> folks seem not to care that it can be triggered?
> 
> I *think* these can be triggered today.  I think it takes two threads
> that do something like:
> 
> 	Thread 1			Thread 2
> 	========			========
> 	ptr = malloc();
> 	memcpy(ptr, &code, len);
> 	exec_now = 1;
> 					while (!exec_now);
> 					call(ptr);		
> 					// fault	
> 	mprotect(ptr, PROT_EXEC, len);
> 					// fault sees VM_EXEC
> 
> 
> But that has a bug: exec_now is set before the mprotect().  It's not
> sane code.
> 
> Can any sane code trigger this?

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.

But I will not bet my life on it. That’s the reason for the somewhat
vague phrasing that I used.

>> 
>> 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;
>> 
>> -	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)) ==
>> +					(X86_PF_WRITE|X86_PF_INSTR)) {
>> +			WARN_ON_ONCE(1);
>> +			return 1;
>> +		}
> 
> 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.

I can wrap it in a different function and also call it from
do_kern_addr_fault() or spurious_kernel_fault().

Anyhow, spurious_kernel_fault() should handle spurious faults on
executable code correctly. 

> 
> 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.

I thought I manage to convey exactly that in the comment. Can you provide
a better phrasing?

> 
>> 		/* write, present and write, not present: */
>> -		if (unlikely(!(vma->vm_flags & VM_WRITE)))
>> +		if ((error_code & X86_PF_WRITE) &&
>> +		    unlikely(!(vma->vm_flags & VM_WRITE)))
>> +			return 1;
>> +
>> +		/* exec, present and exec, not present: */
>> +		if ((error_code & X86_PF_INSTR) &&
>> +		    unlikely(!(vma->vm_flags & VM_EXEC)))
>> 			return 1;
>> +
>> 		return 0;
>> 	}
> 
> This is getting really ugly.  I think we've gone over this before, but
> it escapes me.  Why do we need a common (X86_PF_WRITE | X86_PF_INSTR)
> block of code?  Why can't we just add a simple X86_PF_INSN if() that
> mirrors the current X86_PF_WRITE one?
> 
> 
>        if (error_code & X86_PF_INSN) {
>                /* present and not exec: */
>                if (unlikely(!(vma->vm_flags & VM_EXEC)))
>                        return 1;
>                return 0;
>        }

You are correct. My bad. I will fix it.