From: Nick Desaulniers <nick.desaulniers@gmail.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Nick Desaulniers <nick.desaulniers@gmail.com>,
Shuah Khan <shuah@kernel.org>,
Deepa Dinamani <deepa.kernel@gmail.com>,
Ingo Molnar <mingo@kernel.org>, Al Viro <viro@zeniv.linux.org.uk>,
Frederic Weisbecker <fweisbec@gmail.com>,
Dimitri Sivanich <sivanich@hpe.com>,
linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org
Subject: [PATCH] posix-timers: prevent UB from shifting negative signed value
Date: Thu, 28 Dec 2017 22:01:10 -0500 [thread overview]
Message-ID: <1514516474-15811-1-git-send-email-nick.desaulniers@gmail.com> (raw)
In-Reply-To: <alpine.DEB.2.20.1712281645540.1899@nanos>
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>
---
include/linux/posix-timers.h | 25 +++++++++++++++++++------
kernel/time/posix-clock.c | 2 +-
kernel/time/posix-cpu-timers.c | 4 ++--
tools/testing/selftests/ptp/testptp.c | 4 +---
4 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index 672c4f3..b3d87c0 100644
--- 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))
+inline int fd_to_clockid(const int fd)
+{
+ return (int) make_process_cpuclock((unsigned int) fd, CLOCKFD);
+}
+
+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 17cdc55..cc91d90 100644
--- 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 1f27887a..cef79ca 100644
--- 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 5d2eae1..a5d8f0a 100644
--- 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)
--
2.7.4
next prev parent reply other threads:[~2017-12-29 3:01 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-24 3:26 precedence bug in MAKE_PROCESS_CPUCLOCK macro? Nick Desaulniers
2017-12-28 15:49 ` Thomas Gleixner
2017-12-29 3:01 ` Nick Desaulniers [this message]
2017-12-29 3:08 ` [PATCH] posix-timers: prevent UB from shifting negative signed value Nick Desaulniers
2017-12-29 3:11 ` [PATCH v2] " Nick Desaulniers
2018-01-04 14:01 ` [tip:timers/core] posix-timers: Prevent " tip-bot for Nick Desaulniers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1514516474-15811-1-git-send-email-nick.desaulniers@gmail.com \
--to=nick.desaulniers@gmail.com \
--cc=deepa.kernel@gmail.com \
--cc=fweisbec@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=shuah@kernel.org \
--cc=sivanich@hpe.com \
--cc=tglx@linutronix.de \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).