[PATCH] arm/xen: Enable user access to the kernel before issuing a privcmd call

[PATCH] arm/xen: Enable user access to the kernel before issuing a privcmd call

[Julien Grall]

When Xen is copyin data to/from the guest it will check if the kernel
has the right to do the access. If not, the hypercall will return an
error.

After the commit a5e090acbf545c0a3b04080f8a488b17ec41fe02 "ARM:
software-based priviledged-no-access support", the kernel can't access
anymore the user space by default. This will result to fail on every
hypercall made by the userspace (i.e via privcmd).

We have to enable the userspace access and then restore the correct
permission everytime the privmcd is used to made an hypercall.

I didn't find generic helpers to do a these operations, so the change
is only arm32 specific.

Reported-by: Riku Voipio <riku.voipio@linaro.org>
Signed-off-by: Julien Grall <julien.grall@citrix.com>

---
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Russell King <linux@arm.linux.org.uk>

    ARM64 doesn't seem to have priviledge no-access support yet so there
    is nothing to do for now.

    I haven't look x86 at all.
---
 arch/arm/xen/Makefile    |  1 +
 arch/arm/xen/hypercall.S |  4 ++--
 arch/arm/xen/privcmd.c   | 25 +++++++++++++++++++++++++
 3 files changed, 28 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/xen/privcmd.c

diff --git a/arch/arm/xen/Makefile b/arch/arm/xen/Makefile
index 1296952..d8d088a 100644
--- a/arch/arm/xen/Makefile
+++ b/arch/arm/xen/Makefile
@@ -1 +1,2 @@
 obj-y		:= enlighten.o hypercall.o grant-table.o p2m.o mm.o
+obj-y		+= privcmd.o
diff --git a/arch/arm/xen/hypercall.S b/arch/arm/xen/hypercall.S
index f00e080..56e7181 100644
--- a/arch/arm/xen/hypercall.S
+++ b/arch/arm/xen/hypercall.S
@@ -91,7 +91,7 @@ HYPERCALL3(vcpu_op);
 HYPERCALL1(tmem_op);
 HYPERCALL2(multicall);
 
-ENTRY(privcmd_call)
+ENTRY(__privcmd_call)
 	stmdb sp!, {r4}
 	mov r12, r0
 	mov r0, r1
@@ -102,4 +102,4 @@ ENTRY(privcmd_call)
 	__HVC(XEN_IMM)
 	ldm sp!, {r4}
 	ret lr
-ENDPROC(privcmd_call);
+ENDPROC(__privcmd_call);
diff --git a/arch/arm/xen/privcmd.c b/arch/arm/xen/privcmd.c
new file mode 100644
index 0000000..97f502a
--- /dev/null
+++ b/arch/arm/xen/privcmd.c
@@ -0,0 +1,25 @@
+#include <linux/uaccess.h>
+#include <asm/xen/hypervisor.h>
+
+/* Forward declaration for the assembly function living in hypercall.S */
+long __privcmd_call(unsigned call, unsigned int long a1,
+		    unsigned long a2, unsigned long a3,
+		    unsigned long a4, unsigned long a5);
+
+long privcmd_call(unsigned call, unsigned int long a1,
+		  unsigned long a2, unsigned long a3,
+		  unsigned long a4, unsigned long a5)
+{
+	long ret;
+	/*
+	 * Privcmd calls are issued by the userspace. We need to allow the
+	 * kernel to access the userspace memory before issuing the hypercall.
+	 */
+	unsigned int ua_flags = uaccess_save_and_enable();
+
+	ret = __privcmd_call(call, a1, a2, a3, a4, a5);
+
+	uaccess_restore(ua_flags);
+
+	return ret;
+}
-- 
2.1.4

Re: [PATCH] arm/xen: Enable user access to the kernel before issuing a privcmd call

[Ian Campbell]

On Fri, 2015-09-11 at 15:16 +0100, Julien Grall wrote:
> When Xen is copyin data to/from the guest it will check if the kernel

"copying"

> has the right to do the access. If not, the hypercall will return an
> error.
> 
> After the commit a5e090acbf545c0a3b04080f8a488b17ec41fe02 "ARM:
> software-based priviledged-no-access support", the kernel can't access

"privileged"

> anymore the user space by default. This will result to fail on every

"any more" (or "any longer")

> hypercall made by the userspace (i.e via privcmd).
> 
> We have to enable the userspace access and then restore the correct
> permission everytime the privmcd is used to made an hypercall.

"every time" and "privcmd"

>  HYPERCALL1(tmem_op);
>  HYPERCALL2(multicall);
>  
> -ENTRY(privcmd_call)
> +ENTRY(__privcmd_call)

arch/arm/include/asm/assembler.h seems to contain uaccess_* macros which
could be used right here directly I think? That would be preferable to
wrapping I think.

Ian.

Re: [PATCH] arm/xen: Enable user access to the kernel before issuing a privcmd call

[Julien Grall]

On 11/09/15 15:29, Ian Campbell wrote:
> On Fri, 2015-09-11 at 15:16 +0100, Julien Grall wrote:
>> When Xen is copyin data to/from the guest it will check if the kernel
> 
> "copying"
> 
>> has the right to do the access. If not, the hypercall will return an
>> error.
>>
>> After the commit a5e090acbf545c0a3b04080f8a488b17ec41fe02 "ARM:
>> software-based priviledged-no-access support", the kernel can't access
> 
> "privileged"
> 
>> anymore the user space by default. This will result to fail on every
> 
> "any more" (or "any longer")
> 
>> hypercall made by the userspace (i.e via privcmd).
>>
>> We have to enable the userspace access and then restore the correct
>> permission everytime the privmcd is used to made an hypercall.
> 
> "every time" and "privcmd"
> 
>>  HYPERCALL1(tmem_op);
>>  HYPERCALL2(multicall);
>>  
>> -ENTRY(privcmd_call)
>> +ENTRY(__privcmd_call)
> 
> arch/arm/include/asm/assembler.h seems to contain uaccess_* macros which
> could be used right here directly I think? That would be preferable to
> wrapping I think.

Looking to the uaccess_save macro:

        .macro  uaccess_save, tmp
#ifdef CONFIG_CPU_SW_DOMAIN_PAN
        mrc     p15, 0, \tmp, c3, c0, 0
        str     \tmp, [sp, #S_FRAME_SIZE]
#endif
        .endm


It's saving the register on the Stack with an offset S_FRAME_SIZE.
AFAICT, S_FRAME_SIZE is the size of the pt_regs structure.

So it looks like to me that they are unusable for any assembly functions
but entry point.

I though about using a static inline for privcmd_call but it was
introducing changes on the arm64 in order to decouple hypercall.h (it's
common right now).

Regards,

-- 
Julien Grall

Re: [PATCH] arm/xen: Enable user access to the kernel before issuing a privcmd call

[Ian Campbell]

On Fri, 2015-09-11 at 15:45 +0100, Julien Grall wrote:
> On 11/09/15 15:29, Ian Campbell wrote:
> > On Fri, 2015-09-11 at 15:16 +0100, Julien Grall wrote:
> > > When Xen is copyin data to/from the guest it will check if the kernel
> > 
> > "copying"
> > 
> > > has the right to do the access. If not, the hypercall will return an
> > > error.
> > > 
> > > After the commit a5e090acbf545c0a3b04080f8a488b17ec41fe02 "ARM:
> > > software-based priviledged-no-access support", the kernel can't
> > > access
> > 
> > "privileged"
> > 
> > > anymore the user space by default. This will result to fail on every
> > 
> > "any more" (or "any longer")
> > 
> > > hypercall made by the userspace (i.e via privcmd).
> > > 
> > > We have to enable the userspace access and then restore the correct
> > > permission everytime the privmcd is used to made an hypercall.
> > 
> > "every time" and "privcmd"
> > 
> > >  HYPERCALL1(tmem_op);
> > >  HYPERCALL2(multicall);
> > >  
> > > -ENTRY(privcmd_call)
> > > +ENTRY(__privcmd_call)
> > 
> > arch/arm/include/asm/assembler.h seems to contain uaccess_* macros
> > which
> > could be used right here directly I think? That would be preferable to
> > wrapping I think.
> 
> Looking to the uaccess_save macro:

I was thinking more about uaccess_enable/disable.

Ian.

Re: [PATCH] arm/xen: Enable user access to the kernel before issuing a privcmd call

[Julien Grall]

On 11/09/15 15:55, Ian Campbell wrote:
> On Fri, 2015-09-11 at 15:45 +0100, Julien Grall wrote:
>> On 11/09/15 15:29, Ian Campbell wrote:
>>> On Fri, 2015-09-11 at 15:16 +0100, Julien Grall wrote:
>>>> When Xen is copyin data to/from the guest it will check if the kernel
>>>
>>> "copying"
>>>
>>>> has the right to do the access. If not, the hypercall will return an
>>>> error.
>>>>
>>>> After the commit a5e090acbf545c0a3b04080f8a488b17ec41fe02 "ARM:
>>>> software-based priviledged-no-access support", the kernel can't
>>>> access
>>>
>>> "privileged"
>>>
>>>> anymore the user space by default. This will result to fail on every
>>>
>>> "any more" (or "any longer")
>>>
>>>> hypercall made by the userspace (i.e via privcmd).
>>>>
>>>> We have to enable the userspace access and then restore the correct
>>>> permission everytime the privmcd is used to made an hypercall.
>>>
>>> "every time" and "privcmd"
>>>
>>>>  HYPERCALL1(tmem_op);
>>>>  HYPERCALL2(multicall);
>>>>  
>>>> -ENTRY(privcmd_call)
>>>> +ENTRY(__privcmd_call)
>>>
>>> arch/arm/include/asm/assembler.h seems to contain uaccess_* macros
>>> which
>>> could be used right here directly I think? That would be preferable to
>>> wrapping I think.
>>
>> Looking to the uaccess_save macro:
> 
> I was thinking more about uaccess_enable/disable.

Well, we can't assume that the function will be called with uaccess
disabled.  So we have to save the state and restore it after issuing the
hypercall.

Regards,

-- 
Julien Grall

Re: [PATCH] arm/xen: Enable user access to the kernel before issuing a privcmd call

[Russell King - ARM Linux]

On Fri, Sep 11, 2015 at 03:56:38PM +0100, Julien Grall wrote:
> Well, we can't assume that the function will be called with uaccess
> disabled.

Please explain your reasoning.

The reason copy_from_user() et.al. need to save and restore the DACR is
because the DACR may be in one of two states on older ARM architectures.
It may have set the kernel domain to 'manager' mode, to allow these
accessors to work on kernel memory, or the kernel domain may be in
'client' mode, thereby preventing the accessors from touching kernel
memory.

Unless the code path is reachable with the kernel domain in manager
mode, (iow, a set_fs(KERNEL_DS) or set_fs(get_ds()) has been done) then
it should be safe to use uaccess_disable/uaccess_enable.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] arm/xen: Enable user access to the kernel before issuing a privcmd call

[Julien Grall]

On 11/09/15 16:25, Russell King - ARM Linux wrote:
> On Fri, Sep 11, 2015 at 03:56:38PM +0100, Julien Grall wrote:
>> Well, we can't assume that the function will be called with uaccess
>> disabled.
> 
> Please explain your reasoning.

I think I was confused about the usage of uaccess.
Thank you for the explanation.

> The reason copy_from_user() et.al. need to save and restore the DACR is
> because the DACR may be in one of two states on older ARM architectures.
> It may have set the kernel domain to 'manager' mode, to allow these
> accessors to work on kernel memory, or the kernel domain may be in
> 'client' mode, thereby preventing the accessors from touching kernel
> memory.
> 
> Unless the code path is reachable with the kernel domain in manager
> mode, (iow, a set_fs(KERNEL_DS) or set_fs(get_ds()) has been done) then
> it should be safe to use uaccess_disable/uaccess_enable.


I believe that our privcmd driver doesn't made any usage of set_fs(...),
so I will use the uaccess_{disable,enable} macro.

Regards,

-- 
Julien Grall

Re: [PATCH] arm/xen: Enable user access to the kernel before issuing a privcmd call

[Russell King - ARM Linux]

On Fri, Sep 11, 2015 at 03:45:29PM +0100, Julien Grall wrote:
> Looking to the uaccess_save macro:
> 
>         .macro  uaccess_save, tmp
> #ifdef CONFIG_CPU_SW_DOMAIN_PAN
>         mrc     p15, 0, \tmp, c3, c0, 0
>         str     \tmp, [sp, #S_FRAME_SIZE]
> #endif
>         .endm
> 
> 
> It's saving the register on the Stack with an offset S_FRAME_SIZE.
> AFAICT, S_FRAME_SIZE is the size of the pt_regs structure.

While in the kernel, the uaccess state will be disabled except for the
specific sites reading or writing userspace.  That means the user-access
part of the DACR is well known.

What isn't known is whether we're executing here in the kernel segment
(set_fs(get_ds())) or not - which allows the kernel to use the userspace
accessors to safely read from its own memory space.

If we can assume that's not in effect, then the DACR value is fully
known at this point, and you can just do a:

	uaccess_enable rtmp
	HVC call
	uaccess_disable rtmp

where rtmp is a register you can afford to be changed by the macro.
I suspect 'ip' would be a good choice there.

Please put the macros as close to __HVC(XEN_IMM) as possible.

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] arm/xen: Enable user access to the kernel before issuing a privcmd call

[Julien Grall]

Hi Ian,

On 11/09/15 15:29, Ian Campbell wrote:
>> After the commit a5e090acbf545c0a3b04080f8a488b17ec41fe02 "ARM:
>> software-based priviledged-no-access support", the kernel can't access
> 
> "privileged"

That was a typo in the commit title of the patch. So I won't fix this one.

All the others will be fixed on the next version.

Regards,

-- 
Julien Grall