cxl: fix setting of _PAGE_USER bit when handling page faults

[Aneesh Kumar K.V]

Michael Ellerman <mpe at ellerman.id.au> writes:

> On Mon, 2016-04-11 at 10:01 +0530, Aneesh Kumar K.V wrote:
>> Michael Ellerman <mpe at ellerman.id.au> writes:
>> > On Mon, 2016-04-11 at 14:10 +1000, Andrew Donnellan wrote:
>> > > On 29/03/16 00:42, Aneesh Kumar K.V wrote:
>> > > > I noticed this when doing radix support and have a variant posted at
>> > > > 
>> > > > https://lists.ozlabs.org/pipermail/linuxppc-dev/2016-March/141036.html
>> > > 
>> > > I'm happy for this to be fixed in your radix series.
>> > 
>> > I'm not :)
>> > 
>> > This needs a stand-alone fix that we can backport.
>> 
>> It is done as an independent patch 
>> 
>> http://mid.gmane.org/1460182444-2468-2-git-send-email-aneesh.kumar@linux.vnet.ibm.com
>
> Sure, but that's still not a *fix*.
>
> A fix is a single commit, preferably with a subject that literally contains the
> word "fix" or "bug", which fixes just the bug and nothing else. It should also
> have a Fixes: line, if possible, and a Cc stable if appropriate.
>
> It should also describe clearly what the bug is, why it's serious or just
> annoying or whatever.
>
> 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. Do you agree that the current code won't allow 
"userspace create mappings of kernel memory with  _PAGE_USER set" ?
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.

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 ?

-aneesh