On Fri, Aug 14, 2020 at 11:06:59PM +0200, Mark Kettenis wrote: > > Date: Fri, 14 Aug 2020 14:40:23 +0200 (CEST) > > From: Mark Kettenis <mark.kette...@xs4all.nl> > > > > I suppose a way to test this properly is to pick a system call and > > replace a copyin() with a direct access? That will succeed without > > PAN but should fail with PAN enabled right? > > So that does indeed work. However, the result is a hard hang. So > here as an additional diff that makes sure we panic instead. The idea > is that all user-space access from the kernel should be done by the > special unprivileged load/store instructions.
Would disabling PSTATE.PAN in copyin/copyout along the lines of how stac/clac is done for SMAP avoid having to test the instruction type entirely? Is it faulting on valid copyin/copyout the way you have it at the moment? I don't quite follow what is going on. > > Index: arch/arm64/arm64/trap.c > =================================================================== > RCS file: /cvs/src/sys/arch/arm64/arm64/trap.c,v > retrieving revision 1.27 > diff -u -p -r1.27 trap.c > --- arch/arm64/arm64/trap.c 6 Jan 2020 12:37:30 -0000 1.27 > +++ arch/arm64/arm64/trap.c 14 Aug 2020 21:05:54 -0000 > @@ -65,6 +65,14 @@ void do_el0_error(struct trapframe *); > > void dumpregs(struct trapframe*); > > +/* Check whether we're executing an unprivileged load/store instruction. */ > +static inline int > +is_unpriv_ldst(uint64_t elr) > +{ > + uint32_t insn = *(uint32_t *)elr; > + return ((insn & 0x3f200c00) == 0x38000800); The value of op1 (bit 26) is not used according to the table in the Arm ARM. The mask would be better as 0x3b200c00 > +} > + > static void > data_abort(struct trapframe *frame, uint64_t esr, uint64_t far, > int lower, int exe) > @@ -104,8 +112,18 @@ data_abort(struct trapframe *frame, uint > /* The top bit tells us which range to use */ > if ((far >> 63) == 1) > map = kernel_map; > - else > + else { > + /* > + * Only allow user-space access using > + * unprivileged load/store instructions. > + */ > + if (!is_unpriv_ldst(frame->tf_elr)) { > + panic("attempt to access user address" > + " 0x%llx from EL1", far); > + } > + > map = &p->p_vmspace->vm_map; > + } > } > > if (exe) > >