[PATCH] cxl: fix setting of _PAGE_USER bit when handling page faults

[PATCH] cxl: fix setting of _PAGE_USER bit when handling page faults

[Andrew Donnellan]

When handling page faults, cxl_handle_page_fault() checks whether the page
should be accessible by userspace and have its _PAGE_USER access bit set.
_PAGE_USER should be set if the context's kernel flag isn't set, or if the
page falls outside of kernel memory.

However, the check currently uses the wrong operator, causing it to always
evalute to true. As such, we always set the _PAGE_USER bit, even when it
should be restricted to the kernel.

Fix the check so that the _PAGE_USER bit is set only as intended.

Fixes: f204e0b8cedd ("cxl: Driver code for powernv PCIe based cards for
userspace access")
Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>

---

Found by Coverity Scan.

Currently, this should only affect cxlflash.
---
 drivers/misc/cxl/fault.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/cxl/fault.c b/drivers/misc/cxl/fault.c
index 9a8650b..a76cb8a 100644
--- a/drivers/misc/cxl/fault.c
+++ b/drivers/misc/cxl/fault.c
@@ -152,7 +152,7 @@ static void cxl_handle_page_fault(struct cxl_context *ctx,
 	access = _PAGE_PRESENT;
 	if (dsisr & CXL_PSL_DSISR_An_S)
 		access |= _PAGE_RW;
-	if ((!ctx->kernel) || ~(dar & (1ULL << 63)))
+	if ((!ctx->kernel) || !(dar & (1ULL << 63)))
 		access |= _PAGE_USER;
 
 	if (dsisr & DSISR_NOHPTE)
-- 
Andrew Donnellan              Software Engineer, OzLabs
andrew.donnellan@au1.ibm.com  Australia Development Lab, Canberra
+61 2 6201 8874 (work)        IBM Australia Limited

Re: [PATCH] cxl: fix setting of _PAGE_USER bit when handling page faults

[Ian Munsie]

Excerpts from andrew.donnellan's message of 2016-03-18 15:01:21 +1100:
> Fixes: f204e0b8cedd ("cxl: Driver code for powernv PCIe based cards for
> userspace access")

It doesn't fix that since there was no cxl kernel API support at the
time, so this wasn't a regression - just something we missed when the
kernel api was added (I believe the broken test in the code was a left
over from some early bringup work and would never have been exercised on
an upstream kernel until then).

> Currently, this should only affect cxlflash.

We haven't run into any problems because of this that I am aware of - do
we have a test case for this?

> -    if ((!ctx->kernel) || ~(dar & (1ULL << 63)))
> +    if ((!ctx->kernel) || !(dar & (1ULL << 63)))

Should it be the top two bits?

-Ian

Re: [PATCH] cxl: fix setting of _PAGE_USER bit when handling page faults

[Andrew Donnellan]

On 18/03/16 17:30, Ian Munsie wrote:
> Excerpts from andrew.donnellan's message of 2016-03-18 15:01:21 +1100:
>> Fixes: f204e0b8cedd ("cxl: Driver code for powernv PCIe based cards for
>> userspace access")
>
> It doesn't fix that since there was no cxl kernel API support at the
> time, so this wasn't a regression - just something we missed when the
> kernel api was added (I believe the broken test in the code was a left
> over from some early bringup work and would never have been exercised on
> an upstream kernel until then).

Ah, fair enough - I just looked at what git blame told me. You're right, 
it's not a fix to that commit per se. Happy to drop this tag.

> We haven't run into any problems because of this that I am aware of - do
> we have a test case for this?

I'd be surprised if it caused noticeable problems - the presence of the 
_PAGE_USER bit when it's not necessary shouldn't break anything, as 
opposed to the absence of _PAGE_USER when it is necessary. Not entirely 
sure what the test case would be.

>
>> -    if ((!ctx->kernel) || ~(dar & (1ULL << 63)))
>> +    if ((!ctx->kernel) || !(dar & (1ULL << 63)))
>
> Should it be the top two bits?

benh told me that the top bit should be enough - anything above 0x8000* 
should be kernel space.

-- 
Andrew Donnellan              Software Engineer, OzLabs
andrew.donnellan@au1.ibm.com  Australia Development Lab, Canberra
+61 2 6201 8874 (work)        IBM Australia Limited

Re: cxl: fix setting of _PAGE_USER bit when handling page faults

[Michael Ellerman]

On Fri, 2016-18-03 at 04:01:21 UTC, Andrew Donnellan wrote:
> When handling page faults, cxl_handle_page_fault() checks whether the page
> should be accessible by userspace and have its _PAGE_USER access bit set.
> _PAGE_USER should be set if the context's kernel flag isn't set, or if the
> page falls outside of kernel memory.
> 
> However, the check currently uses the wrong operator, causing it to always
> evalute to true. As such, we always set the _PAGE_USER bit, even when it
> should be restricted to the kernel.
> 
> Fix the check so that the _PAGE_USER bit is set only as intended.
> 
..
> diff --git a/drivers/misc/cxl/fault.c b/drivers/misc/cxl/fault.c
> index 9a8650b..a76cb8a 100644
> --- a/drivers/misc/cxl/fault.c
> +++ b/drivers/misc/cxl/fault.c
> @@ -152,7 +152,7 @@ static void cxl_handle_page_fault(struct cxl_context *ctx,
>  	access = _PAGE_PRESENT;
>  	if (dsisr & CXL_PSL_DSISR_An_S)
>  		access |= _PAGE_RW;
> -	if ((!ctx->kernel) || ~(dar & (1ULL << 63)))
> +	if ((!ctx->kernel) || !(dar & (1ULL << 63)))
>  		access |= _PAGE_USER;

I think you can (should) use is_kernel_addr() for the DAR check.

I'm also slightly worried by that logic in the case of a non-kernel context.

ie. if ctx->kernel is false, we get:

	if (true || !is_kernel_addr(dar))
 		access |= _PAGE_USER;

Which means we just add _PAGE_USER for any address. What am I missing here?

cheers

Re: cxl: fix setting of _PAGE_USER bit when handling page faults

[Ian Munsie]

Excerpts from Michael Ellerman's message of 2016-03-25 05:01:38 -0500:
> I think you can (should) use is_kernel_addr() for the DAR check.
> 
> I'm also slightly worried by that logic in the case of a non-kernel context.
> 
> ie. if ctx->kernel is false, we get:
> 
>     if (true || !is_kernel_addr(dar))
>          access |= _PAGE_USER;
> 
> Which means we just add _PAGE_USER for any address. What am I missing here?

It's been ages since I did a deep dive on the related mm code, so I
don't recall the precise details so take this with a grain of salt, but
if memory serves the call to copro_handle_mm_fault will fail if a user
is trying to access a kernel region since it won't be mapped in the mm.

-Ian

Re: cxl: fix setting of _PAGE_USER bit when handling page faults

[Aneesh Kumar K.V]

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

> [ text/plain ]
> On Fri, 2016-18-03 at 04:01:21 UTC, Andrew Donnellan wrote:
>> When handling page faults, cxl_handle_page_fault() checks whether the page
>> should be accessible by userspace and have its _PAGE_USER access bit set.
>> _PAGE_USER should be set if the context's kernel flag isn't set, or if the
>> page falls outside of kernel memory.
>> 
>> However, the check currently uses the wrong operator, causing it to always
>> evalute to true. As such, we always set the _PAGE_USER bit, even when it
>> should be restricted to the kernel.
>> 
>> Fix the check so that the _PAGE_USER bit is set only as intended.
>> 
> ..
>> diff --git a/drivers/misc/cxl/fault.c b/drivers/misc/cxl/fault.c
>> index 9a8650b..a76cb8a 100644
>> --- a/drivers/misc/cxl/fault.c
>> +++ b/drivers/misc/cxl/fault.c
>> @@ -152,7 +152,7 @@ static void cxl_handle_page_fault(struct cxl_context *ctx,
>>  	access = _PAGE_PRESENT;
>>  	if (dsisr & CXL_PSL_DSISR_An_S)
>>  		access |= _PAGE_RW;
>> -	if ((!ctx->kernel) || ~(dar & (1ULL << 63)))
>> +	if ((!ctx->kernel) || !(dar & (1ULL << 63)))
>>  		access |= _PAGE_USER;
>
> I think you can (should) use is_kernel_addr() for the DAR check.
>
> I'm also slightly worried by that logic in the case of a non-kernel context.
>
> ie. if ctx->kernel is false, we get:
>
> 	if (true || !is_kernel_addr(dar))
>  		access |= _PAGE_USER;
>
> Which means we just add _PAGE_USER for any address. What am I missing here?

I noticed this when doing radix support and have a variant posted at

https://lists.ozlabs.org/pipermail/linuxppc-dev/2016-March/141036.html

-aneesh

Re: cxl: fix setting of _PAGE_USER bit when handling page faults

[Aneesh Kumar K.V]

"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:

> [ text/plain ]
> Michael Ellerman <mpe@ellerman.id.au> writes:
>
>> [ text/plain ]
>> On Fri, 2016-18-03 at 04:01:21 UTC, Andrew Donnellan wrote:
>>> When handling page faults, cxl_handle_page_fault() checks whether the page
>>> should be accessible by userspace and have its _PAGE_USER access bit set.
>>> _PAGE_USER should be set if the context's kernel flag isn't set, or if the
>>> page falls outside of kernel memory.
>>> 
>>> However, the check currently uses the wrong operator, causing it to always
>>> evalute to true. As such, we always set the _PAGE_USER bit, even when it
>>> should be restricted to the kernel.
>>> 
>>> Fix the check so that the _PAGE_USER bit is set only as intended.
>>> 
>> ..
>>> diff --git a/drivers/misc/cxl/fault.c b/drivers/misc/cxl/fault.c
>>> index 9a8650b..a76cb8a 100644
>>> --- a/drivers/misc/cxl/fault.c
>>> +++ b/drivers/misc/cxl/fault.c
>>> @@ -152,7 +152,7 @@ static void cxl_handle_page_fault(struct cxl_context *ctx,
>>>  	access = _PAGE_PRESENT;
>>>  	if (dsisr & CXL_PSL_DSISR_An_S)
>>>  		access |= _PAGE_RW;
>>> -	if ((!ctx->kernel) || ~(dar & (1ULL << 63)))
>>> +	if ((!ctx->kernel) || !(dar & (1ULL << 63)))
>>>  		access |= _PAGE_USER;
>>
>> I think you can (should) use is_kernel_addr() for the DAR check.
>>
>> I'm also slightly worried by that logic in the case of a non-kernel context.
>>
>> ie. if ctx->kernel is false, we get:
>>
>> 	if (true || !is_kernel_addr(dar))
>>  		access |= _PAGE_USER;
>>
>> Which means we just add _PAGE_USER for any address. What am I missing here?
>
> I noticed this when doing radix support and have a variant posted at
>
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2016-March/141036.html
>

My change also keep it similar to __hash_page.

	/*
	 * We need to set the _PAGE_USER bit if MSR_PR is set or if we are
	 * accessing a userspace segment (even from the kernel). We assume
	 * kernel addresses always have the high bit set.
	 */
	if ((msr & MSR_PR) || (REGION_ID(ea) == USER_REGION_ID))

If we need this change to go in this merge window i can respin my patch.

-aneesh

Re: [PATCH] cxl: fix setting of _PAGE_USER bit when handling page faults

[Matthew R. Ochs]

> On Mar 17, 2016, at 11:01 PM, Andrew Donnellan =
<andrew.donnellan@au1.ibm.com> wrote:
>=20
> When handling page faults, cxl_handle_page_fault() checks whether the =
page
> should be accessible by userspace and have its _PAGE_USER access bit =
set.
> _PAGE_USER should be set if the context's kernel flag isn't set, or if =
the
> page falls outside of kernel memory.
>=20
> However, the check currently uses the wrong operator, causing it to =
always
> evalute to true. As such, we always set the _PAGE_USER bit, even when =
it
> should be restricted to the kernel.
>=20
> Fix the check so that the _PAGE_USER bit is set only as intended.
>=20
> Fixes: f204e0b8cedd ("cxl: Driver code for powernv PCIe based cards =
for
> userspace access")
> Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>

Per Ian's suggestion, I went ahead and tried this with cxlflash.

Tested-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>

Re: cxl: fix setting of _PAGE_USER bit when handling page faults

[Andrew Donnellan]

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.

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

Re: cxl: fix setting of _PAGE_USER bit when handling page faults

[Michael Ellerman]

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.

cheers

Re: cxl: fix setting of _PAGE_USER bit when handling page faults

[Aneesh Kumar K.V]

Michael Ellerman <mpe@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

-aneesh

Re: cxl: fix setting of _PAGE_USER bit when handling page faults

[Michael Ellerman]

On Mon, 2016-04-11 at 10:01 +0530, Aneesh Kumar K.V wrote:
> Michael Ellerman <mpe@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.

</grumpy> :)

cheers

Re: cxl: fix setting of _PAGE_USER bit when handling page faults

[Aneesh Kumar K.V]

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

> On Mon, 2016-04-11 at 10:01 +0530, Aneesh Kumar K.V wrote:
>> Michael Ellerman <mpe@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

Re: 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@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