[RFC PATCH 0/4] arm64: vdso: check whether the params is invalid address

[RFC PATCH 0/4] arm64: vdso: check whether the params is invalid address

[Yang Yingliang]

The params of gettimeofday(), clock_gettime() and clock_getres()
is not checked. This will cause segment faults when the address is
invalid. 
E.g. gettimeofday(-1, -1);
     clock_gettime(0, -1);
     clock_getres(0, -1);
This patchset add a macro and use it to check the validation of these
pointer params.


Yang Yingliang (4):
  arm64: vdso: introdce a macro for checking the address
  arm64: vdso: check whether the params of gettimeofday() is valid
  arm64: vdso: check whether the tp pointer is valid in clock_gettime()
  arm64: vdso: check whether the res pointer is valid in clock_getres()

 arch/arm64/kernel/asm-offsets.c       |  3 ++
 arch/arm64/kernel/vdso/gettimeofday.S | 71 +++++++++++++++++++++++++++++------
 2 files changed, 62 insertions(+), 12 deletions(-)

-- 
2.5.0

[RFC PATCH 1/4] arm64: vdso: introdce a macro for checking the address

[Yang Yingliang]

Add a RANGE_OK macro for validation checking of the
address given by user.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 arch/arm64/kernel/asm-offsets.c       |  3 +++
 arch/arm64/kernel/vdso/gettimeofday.S | 21 +++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 574081f..b72c2df 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -98,11 +98,14 @@ int main(void)
   BLANK();
   DEFINE(TVAL_TV_SEC,		offsetof(struct timeval, tv_sec));
   DEFINE(TVAL_TV_USEC,		offsetof(struct timeval, tv_usec));
+  DEFINE(TVAL_SZ,		sizeof(struct timeval));
   DEFINE(TSPEC_TV_SEC,		offsetof(struct timespec, tv_sec));
   DEFINE(TSPEC_TV_NSEC,		offsetof(struct timespec, tv_nsec));
+  DEFINE(TSPEC_SZ,		sizeof(struct timespec));
   BLANK();
   DEFINE(TZ_MINWEST,		offsetof(struct timezone, tz_minuteswest));
   DEFINE(TZ_DSTTIME,		offsetof(struct timezone, tz_dsttime));
+  DEFINE(TZ_SZ,			sizeof(struct timezone));
   BLANK();
 #ifdef CONFIG_KVM_ARM_HOST
   DEFINE(VCPU_CONTEXT,		offsetof(struct kvm_vcpu, arch.ctxt));
diff --git a/arch/arm64/kernel/vdso/gettimeofday.S b/arch/arm64/kernel/vdso/gettimeofday.S
index efa79e8..05ccaca 100644
--- a/arch/arm64/kernel/vdso/gettimeofday.S
+++ b/arch/arm64/kernel/vdso/gettimeofday.S
@@ -21,10 +21,31 @@
 #include <linux/linkage.h>
 #include <asm/asm-offsets.h>
 #include <asm/unistd.h>
+#include <asm/memory.h>
 
 #define NSEC_PER_SEC_LO16	0xca00
 #define NSEC_PER_SEC_HI16	0x3b9a
 
+#define USER_DS		TASK_SIZE_64
+/*
+ * Test whether a block of memory is a valid user space address.
+ * Returns 1 if the range is valid, 0 otherwise.
+ *
+ * This is equivalent to the following test:
+ * (u65)addr + (u65)size <= USER_DS
+ *
+ * This needs 65-bit arithmetic.
+ *
+ * x##n - addr
+ * sz - size
+ * x3 - USER_DS
+ * x4 - return value
+ */
+#define RANGE_OK(n, sz)	mov	x3, #USER_DS;		\
+			adds	x4, x##n, sz;		\
+			ccmp	x4, x3, #0x2, cc;	\
+			cset	x4, ls
+
 vdso_data	.req	x6
 use_syscall	.req	w7
 seqcnt		.req	w8
-- 
2.5.0

[RFC PATCH 2/4] arm64: vdso: check whether the params of gettimeofday() is valid

[Yang Yingliang]

When the params of gettimeofday() is an invalid addr(E.g.
gettimeofday(-1, -1)), it will get segment fault. To avoid
this fault, use RANGE_OK to test whether a block of memory
is valid. Returns -EFAULT if the range is invalid, 0 otherwise.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 arch/arm64/kernel/vdso/gettimeofday.S | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/vdso/gettimeofday.S b/arch/arm64/kernel/vdso/gettimeofday.S
index 05ccaca..43ec321 100644
--- a/arch/arm64/kernel/vdso/gettimeofday.S
+++ b/arch/arm64/kernel/vdso/gettimeofday.S
@@ -22,6 +22,7 @@
 #include <asm/asm-offsets.h>
 #include <asm/unistd.h>
 #include <asm/memory.h>
+#include <asm-generic/errno-base.h>
 
 #define NSEC_PER_SEC_LO16	0xca00
 #define NSEC_PER_SEC_HI16	0x3b9a
@@ -78,10 +79,12 @@ ENTRY(__kernel_gettimeofday)
 	/* Acquire the sequence counter and get the timespec. */
 	adr	vdso_data, _vdso_data
 1:	seqcnt_acquire
-	cbnz	use_syscall, 4f
+	cbnz	use_syscall, 5f
 
 	/* If tv is NULL, skip to the timezone code. */
 	cbz	x0, 2f
+	RANGE_OK(0, #TVAL_SZ)
+	cbz	x4, 4f
 	bl	__do_get_tspec
 	seqcnt_check w9, 1b
 
@@ -93,12 +96,18 @@ ENTRY(__kernel_gettimeofday)
 2:
 	/* If tz is NULL, return 0. */
 	cbz	x1, 3f
+	RANGE_OK(1, #TZ_SZ)
+	cbz	x4, 4f
 	ldp	w4, w5, [vdso_data, #VDSO_TZ_MINWEST]
 	stp	w4, w5, [x1, #TZ_MINWEST]
 3:
 	mov	x0, xzr
 	ret	x2
 4:
+	/* tz is invalid */
+	mov	x0, #-EFAULT
+	ret	x2
+5:
 	/* Syscall fallback. */
 	mov	x8, #__NR_gettimeofday
 	svc	#0
-- 
2.5.0

[RFC PATCH 3/4] arm64: vdso: check whether the tp pointer is valid in clock_gettime()

[Yang Yingliang]

Use RANGE_OK to check whether the tp pointer is valid. Returns
-EINVAL if the tp pointer is NULL, returns -EFAULT if it's invalid,
otherwise return 0.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 arch/arm64/kernel/vdso/gettimeofday.S | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/vdso/gettimeofday.S b/arch/arm64/kernel/vdso/gettimeofday.S
index 43ec321..1e377ac 100644
--- a/arch/arm64/kernel/vdso/gettimeofday.S
+++ b/arch/arm64/kernel/vdso/gettimeofday.S
@@ -118,6 +118,9 @@ ENDPROC(__kernel_gettimeofday)
 /* int __kernel_clock_gettime(clockid_t clock_id, struct timespec *tp); */
 ENTRY(__kernel_clock_gettime)
 	.cfi_startproc
+	cbz	x1, 7f
+	RANGE_OK(1, #TSPEC_SZ)
+	cbz     x4, 8f
 	cmp	w0, #CLOCK_REALTIME
 	ccmp	w0, #CLOCK_MONOTONIC, #0x4, ne
 	b.ne	2f
@@ -128,7 +131,7 @@ ENTRY(__kernel_clock_gettime)
 	/* Get kernel timespec. */
 	adr	vdso_data, _vdso_data
 1:	seqcnt_acquire
-	cbnz	use_syscall, 7f
+	cbnz	use_syscall, 9f
 
 	bl	__do_get_tspec
 	seqcnt_check w9, 1b
@@ -148,7 +151,7 @@ ENTRY(__kernel_clock_gettime)
 2:
 	cmp	w0, #CLOCK_REALTIME_COARSE
 	ccmp	w0, #CLOCK_MONOTONIC_COARSE, #0x4, ne
-	b.ne	8f
+	b.ne	10f
 
 	/* xtime_coarse_nsec is already right-shifted */
 	mov	x12, #0
@@ -192,9 +195,15 @@ ENTRY(__kernel_clock_gettime)
 	stp	x10, x11, [x1, #TSPEC_TV_SEC]
 	mov	x0, xzr
 	ret
-7:
+7:	/* tp is NULL */
+	mov	x0, #-EINVAL
+	ret
+8:	/* tp is invalid */
+	mov	x0, #-EFAULT
+	ret
+9:
 	mov	x30, x2
-8:	/* Syscall fallback. */
+10:	/* Syscall fallback. */
 	mov	x8, #__NR_clock_gettime
 	svc	#0
 	ret
-- 
2.5.0

[RFC PATCH 4/4] arm64: vdso: check whether the res pointer is valid in clock_getres()

[Yang Yingliang]

Use RANGE_OK to check whether the res pointer is valid. Returns
-EINVAL if the it's NULL, returns -EFAULT if it's invalid, otherwise
return 0.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 arch/arm64/kernel/vdso/gettimeofday.S | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kernel/vdso/gettimeofday.S b/arch/arm64/kernel/vdso/gettimeofday.S
index 1e377ac..d7e4341 100644
--- a/arch/arm64/kernel/vdso/gettimeofday.S
+++ b/arch/arm64/kernel/vdso/gettimeofday.S
@@ -217,28 +217,36 @@ ENTRY(__kernel_clock_getres)
 	ccmp	w0, #CLOCK_MONOTONIC, #0x4, ne
 	b.ne	1f
 
-	ldr	x2, 5f
+	ldr	x2, 6f
 	b	2f
 1:
 	cmp	w0, #CLOCK_REALTIME_COARSE
 	ccmp	w0, #CLOCK_MONOTONIC_COARSE, #0x4, ne
-	b.ne	4f
-	ldr	x2, 6f
+	b.ne	5f
+	ldr	x2, 7f
 2:
 	cbz	w1, 3f
+	RANGE_OK(1, #TSPEC_SZ)
+	cbz	x4, 4f
 	stp	xzr, x2, [x1]
+	mov     w0, wzr
+	ret
 
 3:	/* res == NULL. */
-	mov	w0, wzr
+	mov	x0, #-EINVAL
 	ret
 
-4:	/* Syscall fallback. */
+4:	/* res is invalid. */
+	mov	x0, #-EFAULT
+	ret
+
+5:	/* Syscall fallback. */
 	mov	x8, #__NR_clock_getres
 	svc	#0
 	ret
-5:
-	.quad	CLOCK_REALTIME_RES
 6:
+	.quad	CLOCK_REALTIME_RES
+7:
 	.quad	CLOCK_COARSE_RES
 	.cfi_endproc
 ENDPROC(__kernel_clock_getres)
-- 
2.5.0

[RFC PATCH 0/4] arm64: vdso: check whether the params is invalid address

[Will Deacon]

On Wed, Jun 01, 2016 at 11:05:58AM +0800, Yang Yingliang wrote:
> The params of gettimeofday(), clock_gettime() and clock_getres()
> is not checked. This will cause segment faults when the address is
> invalid. 
> E.g. gettimeofday(-1, -1);
>      clock_gettime(0, -1);
>      clock_getres(0, -1);
> This patchset add a macro and use it to check the validation of these
> pointer params.

Is this really necessary? It doesn't look to me like other architectures
bother with this, and you're just adding code to the fastpath to handle
cases that shouldn't happen to start with.

Also, I think I remember seeing SEGVs from libc wrappers for other syscalls
in the past, so this just isn't compelling to me.

Will

[RFC PATCH 0/4] arm64: vdso: check whether the params is invalid address

[Yang Yingliang]

On 2016/6/7 1:12, Will Deacon wrote:
> On Wed, Jun 01, 2016 at 11:05:58AM +0800, Yang Yingliang wrote:
>> The params of gettimeofday(), clock_gettime() and clock_getres()
>> is not checked. This will cause segment faults when the address is
>> invalid.
>> E.g. gettimeofday(-1, -1);
>>       clock_gettime(0, -1);
>>       clock_getres(0, -1);
>> This patchset add a macro and use it to check the validation of these
>> pointer params.
>
> Is this really necessary? It doesn't look to me like other architectures
> bother with this, and you're just adding code to the fastpath to handle
> cases that shouldn't happen to start with.
I agree.

I find clock_gettime() don't check if tp is NULL. I think it need a
checking like clock_getres().

What's your suggestion ?

Thanks,
Yang
>
> Also, I think I remember seeing SEGVs from libc wrappers for other syscalls
> in the past, so this just isn't compelling to me.
>
> Will
>
> .
>

[RFC PATCH 0/4] arm64: vdso: check whether the params is invalid address

[Will Deacon]

On Wed, Jun 08, 2016 at 05:18:26PM +0800, Yang Yingliang wrote:
> 
> 
> On 2016/6/7 1:12, Will Deacon wrote:
> >On Wed, Jun 01, 2016 at 11:05:58AM +0800, Yang Yingliang wrote:
> >>The params of gettimeofday(), clock_gettime() and clock_getres()
> >>is not checked. This will cause segment faults when the address is
> >>invalid.
> >>E.g. gettimeofday(-1, -1);
> >>      clock_gettime(0, -1);
> >>      clock_getres(0, -1);
> >>This patchset add a macro and use it to check the validation of these
> >>pointer params.
> >
> >Is this really necessary? It doesn't look to me like other architectures
> >bother with this, and you're just adding code to the fastpath to handle
> >cases that shouldn't happen to start with.
> I agree.
> 
> I find clock_gettime() don't check if tp is NULL. I think it need a
> checking like clock_getres().
> 
> What's your suggestion ?

So what? If you pass NULL then you get a SEGV. Just like if you pass a
NULL pointer to various glibc wrappers.

Don't pass NULL. Problem solved.

Will