diff mbox

[13/19] y2038: add compat handling for sys_semtimedop

Message ID 5686498.JnFHvQTH21@wuerfel
State Not applicable
Headers

Commit Message

Arnd Bergmann May 16, 2015, 7:54 p.m. UTC 
  On Saturday 16 May 2015 00:46:44 Thomas Gleixner wrote:
> On Wed, 6 May 2015, Arnd Bergmann wrote:
> > +SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
> > +		unsigned, nsops,
> > +		const struct __kernel_timespec  __user *, timeout)
> > +{
> > +	unsigned long jiffies_left = 0;
> > +
> > +	if (timeout) {
> > +		struct timespec64 _timeout;
> > +		if (get_timespec64(&_timeout, timeout))
> 
> Moo. I had to look 3 times to get not confused by the extra
> underscore. What's wrong with a proper variable name which is easy to
> distinguish?
> 
> > +			return -EFAULT;
> 
> > +		if (_timeout.tv_sec < 0 || _timeout.tv_nsec < 0 ||
> > +			_timeout.tv_nsec >= 1000000000L)
> > +			return -EINVAL;
> 
> We have proper helper functions to validate time specs.

I ended up fixing both issues you noticed in the same patch
after all, and also simplified it slightly more.

Finally, I also noticed that I had not done a timespec64_to_jiffies()
call at the time when I wrote this patch, but it actually exists now,
so I've reordered my series and am using it in the new version, as
I should have done to start with.

	Arnd

8<----

From e04b14d49273c27d92f1799233b82bcdafb43d9a Mon Sep 17 00:00:00 2001
From: Arnd Bergmann <arnd@arndb.de>
Date: Mon, 27 Apr 2015 23:30:39 +0200
Subject: [UPDATED PATCH] y2038: add compat handling for sys_semtimedop

This moves the compat_sys_semtimedop function to ipc/sem.c so it
can be shared with 32-bit architectures efficiently. Instead of
copying the timespec back to user space, we take a shortcut and
pass the kernel timespec64 value to the low-level implementation
directly.

The native sys_semtimedop() function is modified to take a
__kernel_timespec structure, which will be based on a 64-bit
time_t in the future.

There is a small API change here: if multiple errors are present,
and the timespec argument is an invalid pointer, we now return
-EFAULT before checking any of the other error conditions.
This is what the compat version has always done, but if it is a
problem, we need a more sophisticated approach.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Comments

Thomas Gleixner May 19, 2015, 9:19 a.m. UTC | #1 
On Sat, 16 May 2015, Arnd Bergmann wrote:
> On Saturday 16 May 2015 00:46:44 Thomas Gleixner wrote:
> > On Wed, 6 May 2015, Arnd Bergmann wrote:
> > > +SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
> > > +		unsigned, nsops,
> > > +		const struct __kernel_timespec  __user *, timeout)
> > > +{
> > > +	unsigned long jiffies_left = 0;
> > > +
> > > +	if (timeout) {
> > > +		struct timespec64 _timeout;
> > > +		if (get_timespec64(&_timeout, timeout))
> > 
> > Moo. I had to look 3 times to get not confused by the extra
> > underscore. What's wrong with a proper variable name which is easy to
> > distinguish?
> > 
> > > +			return -EFAULT;
> > 
> > > +		if (_timeout.tv_sec < 0 || _timeout.tv_nsec < 0 ||
> > > +			_timeout.tv_nsec >= 1000000000L)
> > > +			return -EINVAL;
> > 
> > We have proper helper functions to validate time specs.
> 
> I ended up fixing both issues you noticed in the same patch
> after all, and also simplified it slightly more.
> 
> Finally, I also noticed that I had not done a timespec64_to_jiffies()
> call at the time when I wrote this patch, but it actually exists now,
> so I've reordered my series and am using it in the new version, as
> I should have done to start with.

Indeed. I didn't notice either.
 
> >From e04b14d49273c27d92f1799233b82bcdafb43d9a Mon Sep 17 00:00:00 2001
> From: Arnd Bergmann <arnd@arndb.de>
> Date: Mon, 27 Apr 2015 23:30:39 +0200
> Subject: [UPDATED PATCH] y2038: add compat handling for sys_semtimedop
> 
> This moves the compat_sys_semtimedop function to ipc/sem.c so it
> can be shared with 32-bit architectures efficiently. Instead of
> copying the timespec back to user space, we take a shortcut and
> pass the kernel timespec64 value to the low-level implementation
> directly.
> 
> The native sys_semtimedop() function is modified to take a
> __kernel_timespec structure, which will be based on a 64-bit
> time_t in the future.
> 
> There is a small API change here: if multiple errors are present,
> and the timespec argument is an invalid pointer, we now return
> -EFAULT before checking any of the other error conditions.
> This is what the compat version has always done, but if it is a
> problem, we need a more sophisticated approach.

The important part of error checks is that they catch all
cases and combinations. In which order is completely irrelevant.

If something relies on the ordering of error check returns, it's
broken by definition.
 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
diff mbox

Patch

diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index f3fdc312627b..c2a70a8f907d 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -665,7 +665,7 @@  asmlinkage long sys_semop(int semid, struct sembuf __user *sops,
 asmlinkage long sys_semctl(int semid, int semnum, int cmd, unsigned long arg);
 asmlinkage long sys_semtimedop(int semid, struct sembuf __user *sops,
 				unsigned nsops,
-				const struct timespec __user *timeout);
+				const struct __kernel_timespec __user *timeout);
 asmlinkage long sys_shmat(int shmid, char __user *shmaddr, int shmflg);
 asmlinkage long sys_shmget(key_t key, size_t size, int flag);
 asmlinkage long sys_shmdt(char __user *shmaddr);
diff --git a/ipc/compat.c b/ipc/compat.c
index 9b3c85f8a538..2bbdb093d1be 100644
--- a/ipc/compat.c
+++ b/ipc/compat.c
@@ -745,13 +745,3 @@  COMPAT_SYSCALL_DEFINE3(shmctl, int, first, int, second, void __user *, uptr)
 	}
 	return err;
 }
-
-COMPAT_SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsems,
-		       unsigned, nsops,
-		       const struct compat_timespec __user *, timeout)
-{
-	struct timespec __user *ts64;
-	if (compat_convert_timespec(&ts64, timeout))
-		return -EFAULT;
-	return sys_semtimedop(semid, tsems, nsops, ts64);
-}
diff --git a/ipc/sem.c b/ipc/sem.c
index d1a6edd17eba..84d354a34df3 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -72,6 +72,7 @@ 
  *   The worst-case behavior is nevertheless O(N^2) for N wakeups.
  */
 
+#include <linux/compat.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/init.h>
@@ -1779,8 +1780,8 @@  static int get_queue_result(struct sem_queue *q)
 	return error;
 }
 
-SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
-		unsigned, nsops, const struct timespec __user *, timeout)
+static long semtimedop(int semid, struct sembuf __user * tsops,
+		       unsigned nsops, struct timespec64 *timeout)
 {
 	int error = -EINVAL;
 	struct sem_array *sma;
@@ -1809,17 +1810,11 @@  SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
 		goto out_free;
 	}
 	if (timeout) {
-		struct timespec _timeout;
-		if (copy_from_user(&_timeout, timeout, sizeof(*timeout))) {
-			error = -EFAULT;
-			goto out_free;
-		}
-		if (_timeout.tv_sec < 0 || _timeout.tv_nsec < 0 ||
-			_timeout.tv_nsec >= 1000000000L) {
+		if (!timespec64_valid(timeout)) {
 			error = -EINVAL;
 			goto out_free;
 		}
-		jiffies_left = timespec_to_jiffies(&_timeout);
+		jiffies_left = timespec64_to_jiffies(timeout);
 	}
 	max = 0;
 	for (sop = sops; sop < sops + nsops; sop++) {
@@ -2014,10 +2009,36 @@  out_free:
 	return error;
 }
 
+SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
+		unsigned, nsops,
+		const struct __kernel_timespec  __user *, p)
+{
+	struct timespec64 timeout;
+
+	if (p && get_timespec64(&timeout, p))
+		return -EFAULT;
+
+	return semtimedop(semid, tsops, nsops, p ? &timeout : NULL);
+}
+
+#ifdef CONFIG_COMPAT_TIME
+COMPAT_SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
+		unsigned, nsops,
+		const struct compat_timespec  __user *, p)
+{
+	struct timespec64 timeout;
+
+	if (p && compat_get_timespec64(&timeout, p))
+		return -EFAULT;
+
+	return semtimedop(semid, tsops, nsops, p ? &timeout : NULL);
+}
+#endif
+
 SYSCALL_DEFINE3(semop, int, semid, struct sembuf __user *, tsops,
 		unsigned, nsops)
 {
-	return sys_semtimedop(semid, tsops, nsops, NULL);
+	return semtimedop(semid, tsops, nsops, NULL);
 }
 
 /* If CLONE_SYSVSEM is set, establish sharing of SEM_UNDO state between
diff --git a/ipc/syscall.c b/ipc/syscall.c
index 52429489cde0..d7b17355d870 100644
--- a/ipc/syscall.c
+++ b/ipc/syscall.c
@@ -7,6 +7,7 @@ 
 #include <linux/unistd.h>
 
 #ifdef __ARCH_WANT_SYS_IPC
+#include <linux/compat_time.h>
 #include <linux/errno.h>
 #include <linux/ipc.h>
 #include <linux/shm.h>
@@ -26,9 +27,15 @@  SYSCALL_DEFINE6(ipc, unsigned int, call, int, first, unsigned long, second,
 		return sys_semtimedop(first, (struct sembuf __user *)ptr,
 				      second, NULL);
 	case SEMTIMEDOP:
+#if defined(CONFIG_ARCH_HAS_COMPAT_TIME) && !defined(CONFIG_64BIT)
+		return compat_sys_semtimedop(first, (struct sembuf __user *)ptr,
+					     second,
+					     (const struct compat_timespec __user *)fifth);
+#else
 		return sys_semtimedop(first, (struct sembuf __user *)ptr,
 				      second,
 				      (const struct timespec __user *)fifth);
+#endif
 
 	case SEMGET:
 		return sys_semget(first, second, third);