posix-timers: Prevent UB from shifting negative signed value

Shifting a negative signed number is undefined behavior. Looking at the
macros MAKE_PROCESS_CPUCLOCK and FD_TO_CLOCKID, it seems that the
subexpression:

(~(clockid_t) (pid) << 3)

where clockid_t resolves to a signed int, which once negated, is
undefined behavior to shift the value of if the results thus far are
negative.

It was further suggested to make these macros into inline functions.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Dimitri Sivanich <sivanich@hpe.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-kselftest@vger.kernel.org
Cc: Shuah Khan <shuah@kernel.org>
Cc: Deepa Dinamani <deepa.kernel@gmail.com>
Link: https://lkml.kernel.org/r/1514517100-18051-1-git-send-email-nick.desaulniers@gmail.com

diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index 672c4f32311e2f569e99fac56448009d5efbe3ac..c85704fcdbd2189b517f407ad871b02055924df5 100644 (file)
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -42,13 +42,26 @@ struct cpu_timer_list {
 #define CLOCKFD                        CPUCLOCK_MAX
 #define CLOCKFD_MASK           (CPUCLOCK_PERTHREAD_MASK|CPUCLOCK_CLOCK_MASK)
 
-#define MAKE_PROCESS_CPUCLOCK(pid, clock) \
-       ((~(clockid_t) (pid) << 3) | (clockid_t) (clock))
-#define MAKE_THREAD_CPUCLOCK(tid, clock) \
-       MAKE_PROCESS_CPUCLOCK((tid), (clock) | CPUCLOCK_PERTHREAD_MASK)
+static inline clockid_t make_process_cpuclock(const unsigned int pid,
+               const clockid_t clock)
+{
+       return ((~pid) << 3) | clock;
+}
+static inline clockid_t make_thread_cpuclock(const unsigned int tid,
+               const clockid_t clock)
+{
+       return make_process_cpuclock(tid, clock | CPUCLOCK_PERTHREAD_MASK);
+}
 
-#define FD_TO_CLOCKID(fd)      ((~(clockid_t) (fd) << 3) | CLOCKFD)
-#define CLOCKID_TO_FD(clk)     ((unsigned int) ~((clk) >> 3))
+static inline clockid_t fd_to_clockid(const int fd)
+{
+       return make_process_cpuclock((unsigned int) fd, CLOCKFD);
+}
+
+static inline int clockid_to_fd(const clockid_t clk)
+{
+       return ~(clk >> 3);
+}
 
 #define REQUEUE_PENDING 1
 

diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c
index 17cdc554c9fe82c56e9d8a5b68c48715425dcd09..cc91d90abd84d78b4af473e152b7d7d62f85796b 100644 (file)
--- a/kernel/time/posix-clock.c
+++ b/kernel/time/posix-clock.c
@@ -216,7 +216,7 @@ struct posix_clock_desc {
 
 static int get_clock_desc(const clockid_t id, struct posix_clock_desc *cd)
 {
-       struct file *fp = fget(CLOCKID_TO_FD(id));
+       struct file *fp = fget(clockid_to_fd(id));
        int err = -EINVAL;
 
        if (!fp)

diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
index 1f27887aa19487e82950498485bc83a295098104..cef79ca5bbd5fb0d25657cfcaab6fead7be04c08 100644 (file)
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -1363,8 +1363,8 @@ static long posix_cpu_nsleep_restart(struct restart_block *restart_block)
        return do_cpu_nanosleep(which_clock, TIMER_ABSTIME, &t);
 }
 
-#define PROCESS_CLOCK  MAKE_PROCESS_CPUCLOCK(0, CPUCLOCK_SCHED)
-#define THREAD_CLOCK   MAKE_THREAD_CPUCLOCK(0, CPUCLOCK_SCHED)
+#define PROCESS_CLOCK  make_process_cpuclock(0, CPUCLOCK_SCHED)
+#define THREAD_CLOCK   make_thread_cpuclock(0, CPUCLOCK_SCHED)
 
 static int process_cpu_clock_getres(const clockid_t which_clock,
                                    struct timespec64 *tp)

diff --git a/tools/testing/selftests/ptp/testptp.c b/tools/testing/selftests/ptp/testptp.c
index 5d2eae16f7ee5ef9ab2d9ecfc15deed3a3da97dd..a5d8f0ab0da000211fd750b10fef2386ae5cfa4b 100644 (file)
--- a/tools/testing/selftests/ptp/testptp.c
+++ b/tools/testing/selftests/ptp/testptp.c
@@ -60,9 +60,7 @@ static int clock_adjtime(clockid_t id, struct timex *tx)
 static clockid_t get_clockid(int fd)
 {
 #define CLOCKFD 3
-#define FD_TO_CLOCKID(fd)      ((~(clockid_t) (fd) << 3) | CLOCKFD)
-
-       return FD_TO_CLOCKID(fd);
+       return (((unsigned int) ~fd) << 3) | CLOCKFD;
 }
 
 static void handle_alarm(int s)