linux-kernel - Re: [v3 PATCH] arm64: mm: force write fault for atomic RMW instructions

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: <c6894bbc-d3c4-424c-b47e-4b2a2eedc6a0@os.amperecomputing.com>
Date: Wed, 5 Jun 2024 10:12:01 -0700
From: Yang Shi <yang@...amperecomputing.com>
To: Catalin Marinas <catalin.marinas@....com>
Cc: will@...nel.org, anshuman.khandual@....com, scott@...amperecomputing.com,
 cl@...two.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [v3 PATCH] arm64: mm: force write fault for atomic RMW
 instructions



On 6/5/24 9:54 AM, Catalin Marinas wrote:
> On Tue, Jun 04, 2024 at 10:15:16AM -0700, Yang Shi wrote:
>> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
>> index 8c0a36f72d6f..4e0aa6738579 100644
>> --- a/arch/arm64/include/asm/insn.h
>> +++ b/arch/arm64/include/asm/insn.h
>> @@ -325,6 +325,7 @@ static __always_inline u32 aarch64_insn_get_##abbr##_value(void)	\
>>    * "-" means "don't care"
>>    */
>>   __AARCH64_INSN_FUNCS(class_branch_sys,	0x1c000000, 0x14000000)
>> +__AARCH64_INSN_FUNCS(class_atomic,	0x3b200c00, 0x38200000)
>>   
>>   __AARCH64_INSN_FUNCS(adr,	0x9F000000, 0x10000000)
>>   __AARCH64_INSN_FUNCS(adrp,	0x9F000000, 0x90000000)
>> @@ -345,6 +346,7 @@ __AARCH64_INSN_FUNCS(ldeor,	0x3F20FC00, 0x38202000)
>>   __AARCH64_INSN_FUNCS(ldset,	0x3F20FC00, 0x38203000)
>>   __AARCH64_INSN_FUNCS(swp,	0x3F20FC00, 0x38208000)
>>   __AARCH64_INSN_FUNCS(cas,	0x3FA07C00, 0x08A07C00)
>> +__AARCH64_INSN_FUNCS(casp,	0xBFA07C00, 0x08207C00)
>>   __AARCH64_INSN_FUNCS(ldr_reg,	0x3FE0EC00, 0x38606800)
>>   __AARCH64_INSN_FUNCS(signed_ldr_reg, 0X3FE0FC00, 0x38A0E800)
>>   __AARCH64_INSN_FUNCS(ldr_imm,	0x3FC00000, 0x39400000)
>> @@ -549,6 +551,21 @@ static __always_inline bool aarch64_insn_uses_literal(u32 insn)
>>   	       aarch64_insn_is_prfm_lit(insn);
>>   }
>>   
>> +static __always_inline bool aarch64_insn_is_class_cas(u32 insn)
>> +{
>> +	return aarch64_insn_is_cas(insn) ||
>> +	       aarch64_insn_is_casp(insn);
>> +}
>> +
>> +/* Exclude unallocated atomic instructions and LD64B/LDAPR */
>> +static __always_inline bool aarch64_atomic_insn_has_wr_perm(u32 insn)
>> +{
>> +	return (((insn & 0x3f207c00) == 0x38200000) |
>> +		((insn & 0x3f208c00) == 0x38200000) |
>> +		((insn & 0x7fe06c00) == 0x78202000) |
>> +		((insn & 0xbf204c00) == 0x38200000));
> Please use the logical || instead of the bitwise operator. You can also
> remove the outer brackets.

OK

>
> That said, the above is pretty opaque if we want to update it in the
> future. I have no idea how it was generated or whether it's correct. At
> least maybe add a comment on how you got to these masks and values.

It was generated by a script using Python sympy module, which could help 
figure out the most simplified condition.

>
>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>> index 8251e2fea9c7..1ed1b061ee8f 100644
>> --- a/arch/arm64/mm/fault.c
>> +++ b/arch/arm64/mm/fault.c
>> @@ -519,6 +519,35 @@ static bool is_write_abort(unsigned long esr)
>>   	return (esr & ESR_ELx_WNR) && !(esr & ESR_ELx_CM);
>>   }
>>   
>> +static bool is_el0_atomic_instr(struct pt_regs *regs)
>> +{
>> +	u32 insn;
>> +	__le32 insn_le;
>> +	unsigned long pc = instruction_pointer(regs);
>> +
>> +	if (!user_mode(regs) || compat_user_mode(regs))
>> +		return false;
>> +
>> +	pagefault_disable();
>> +	if (get_user(insn_le, (__le32 __user *)pc)) {
>> +		pagefault_enable();
>> +		return false;
>> +	}
>> +	pagefault_enable();
>> +
>> +	insn = le32_to_cpu(insn_le);
>> +
>> +	if (aarch64_insn_is_class_atomic(insn)) {
>> +		if (aarch64_atomic_insn_has_wr_perm(insn))
>> +			return true;
>> +	}
> Nitpick:
>
> 	if (aarch64_insn_is_class_atomic(insn) &&
> 	    aarch64_atomic_insn_has_wr_perm(insn))
> 		return true;
>
> (less indentation)

Sure

>
>> @@ -557,6 +587,11 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
>>   		/* It was write fault */
>>   		vm_flags = VM_WRITE;
>>   		mm_flags |= FAULT_FLAG_WRITE;
>> +	} else if (is_el0_atomic_instr(regs)) {
>> +		/* Force write fault */
>> +		vm_flags = VM_WRITE;
>> +		mm_flags |= FAULT_FLAG_WRITE;
>> +		force_write = true;
>>   	} else {
>>   		/* It was read fault */
>>   		vm_flags = VM_READ;
>> @@ -586,6 +621,14 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
>>   	if (!vma)
>>   		goto lock_mmap;
>>   
>> +	/* vma flags don't allow write, undo force write */
>> +	if (force_write && !(vma->vm_flags & VM_WRITE)) {
>> +		vm_flags |= VM_READ;
>> +		if (!alternative_has_cap_unlikely(ARM64_HAS_EPAN))
>> +			vm_flags |= VM_EXEC;
>> +		mm_flags &= ~FAULT_FLAG_WRITE;
>> +	}
> Ah, this revert to the non-write flags doesn't look great as we
> basically duplicate the 'else' block in the original check. So, it
> probably look better as per your earlier patch to just do the
> instruction read just before the !(vma->vm_flags & flags) check,
> something like:
>
> 	if ((vma->vm_flags & VM_WRITE) && is_el0_atomic_instr(regs)) {
> 		vm_flags = VM_WRITE;
> 		mm_flags |= FAULT_FLAG_WRITE;
> 	}
>
> This way we also only read the instruction if the vma is writeable. I
> think it's fine to do this under the vma lock since we have
> pagefault_disable() for the insn read.

Yes, I agree.
>