Skip to contentSkip to conversation

arm64: vdso: check whether the params is invalid address

RFC PATCH New Yang Yingliang 2 participants 8 messages Jun 1, 2016 – Jun 8, 2016 arm64

lore
Share & tools
lore

Yang Yingliang

author
#0 lore raw

cover arm64: vdso: check whether the params is invalid address

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(-)
Signature · 1 line
2.5.0
author
#1 lore raw

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

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

Signed-off-by Yang Yingliang <[email protected]>
---
 arch/arm64/kernel/asm-offsets.c       |  3 +++
 arch/arm64/kernel/vdso/gettimeofday.S | 21 +++++++++++++++++++++
 2 files changed, 24 insertions(+)
arch/arm64/kernel/asm-offsets.c +3 -0
index 574081f..b72c2df 100644
@@ -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));
arch/arm64/kernel/vdso/gettimeofday.S +21 -0
index efa79e8..05ccaca 100644
@@ -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
Signature · 1 line
2.5.0
author
#2 lore raw

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

In reply to 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 <[email protected]>
---
 arch/arm64/kernel/vdso/gettimeofday.S | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
arch/arm64/kernel/vdso/gettimeofday.S +10 -1
index 05ccaca..43ec321 100644
@@ -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
Signature · 1 line
2.5.0
author
#3 lore raw

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

In reply to 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 <[email protected]>
---
 arch/arm64/kernel/vdso/gettimeofday.S | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)
arch/arm64/kernel/vdso/gettimeofday.S +13 -4
index 43ec321..1e377ac 100644
@@ -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
Signature · 1 line
2.5.0
author
#4 lore raw

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

In reply to 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 <[email protected]>
---
 arch/arm64/kernel/vdso/gettimeofday.S | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
arch/arm64/kernel/vdso/gettimeofday.S +15 -7
index 1e377ac..d7e4341 100644
@@ -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)
Signature · 1 line
2.5.0
#5 lore raw

cover arm64: vdso: check whether the params is invalid address

In reply to Yang Yingliang
Quoted 8 lines · 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.

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

author
#6 lore raw

cover arm64: vdso: check whether the params is invalid address

Quoted 10 lines · Will Deacon
Quoted 8 lines · 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.

Quoted 3 lines

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

Quoted 8 lines

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

.

#7 lore raw

cover arm64: vdso: check whether the params is invalid address

Quoted 16 lines · Yang Yingliang
Quoted 10 lines · Will Deacon
Quoted 8 lines · 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.

Quoted 3 lines

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.

Quoted 6 lines

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