cxl: fix setting of _PAGE_USER bit when handling page faults

[Michael Ellerman]

On Mon, 2016-04-11 at 19:12 +0530, Aneesh Kumar K.V wrote:
> Michael Ellerman <mpe at ellerman.id.au> writes:
> > 
> > In this case it *looks* like we have a giant hole in the mm handling for CAPI
> > contexts, which would let userspace create mappings of kernel memory with
> > _PAGE_USER set. I think I agree with Ian that in fact that's not true, but it's
> > not clear from the diff that is the case. So I'd really like someone to write a
> > good commit message demonstrating that we understand what the bug is and why
> > it's not a big deal, despite the patch looking scary at first glance.
> 
> That confused me.

Sorry :)

> Do you agree that the current code won't allow 
> "userspace create mappings of kernel memory with  _PAGE_USER set" ?

Yes. My point is that the diff doesn't make that clear - and at first glance it
looks like it could be a bad bug. So it needs a good change log explaning
why it's not possible.

> Or are you suggesting that we do and this need to be documented ?
> 
> If it is later, that is not true. The current code will set _PAGE_USER
> to the access flags for any fault address. ie, because ~ operation will
> be true for all address we take fault on. But setting _PAGE_USER also means
> that the fault will be handled only if the page table have _PAGE_USER
> set.
> 
> Now if it is an user space access, then the change really don't have an
> impact because we have (!ctx->kernel) true for that case and we take
> that if condition true.

Right. And if it was a userspace access of a kernel address it should never have
got that far, because copro_handle_mm_fault() should have failed IIUIC.

> 
> Now if kernel is faulting, which I am not sure capi can result such a
> fault and it is faulting on a adress in the kernel range, then the
> current code will result in a loop fault, because we will not insert
> hash pte due to access and pte permission mismatch. So there is
> no security hole in the fault handling AFAIU.
> 
> Are you suggesting that the above should be documented in the commit
> message ?

Yep.

cheers