On Sat, Aug 15, 2020 at 01:54:34PM +0200, Mark Kettenis wrote:
> > Date: Sat, 15 Aug 2020 20:21:09 +1000
> > From: Jonathan Gray <j...@jsg.id.au>
> >
> > 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?
>
> No. The problem is that we meed to catch permission faults caused by
> PAN. But since the faulting address may be valid in the sense that
> UVM has a mapping for them that allows the requested access. In that
> case we end up looping since uvm_fault() returns 0 and we retry the
> faulting instruction.
>
> > Is it faulting on valid copyin/copyout the way you have it at the
> > moment? I don't quite follow what is going on.
>
> The copyin/copyout functions use the unpriviliged load/store
> instructions (LDTR/STTR) which bypass PAN. But we may still fault
> because the memory hasn't been faulted in or because the memory has
> been marked read-only for CoW or for MOD/REF emulation. And some of
> those faults manifest themselves as permission faults as well.
>
> Currently (without the diff quoted below) those faults will be handled
> just fine. The diff below needs to make sure this continues to be the
> case. The easiest way to do that is to check the instruction.
>
> Note that this check is in the "slow path". In most cases the address
> touched by copyin/copyout will already be in the page tables since
> userland touched it already.
>
> Does that clarfify things?
Yes, thanks. I'm fine with both of these diffs going in but still think
you should change the mask.
>
>
> > > 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)
> > >
> > >
> >
>
>
