On Mon, 2016-04-11 at 19:12 +0530, Aneesh Kumar K.V wrote:

Michael Ellerman [off-list ref] writes:

quoted

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