linux-kernel - precedence bug in MAKE_PROCESS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAH7mPvgzy+DRUoqNMnLqSLQZaWquodTmUe3=m3ZG=CqpL7cBzw@mail.gmail.com>
Date:   Sat, 23 Dec 2017 22:26:01 -0500
From:   Nick Desaulniers <nick.desaulniers@...il.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Al Viro <viro@...iv.linux.org.uk>,
        Deepa Dinamani <deepa.kernel@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: precedence bug in MAKE_PROCESS_CPUCLOCK macro?

I'm seeing the following warning compiling with Clang:

kernel/time/posix-cpu-timers.c:1397:29: warning: shifting a negative
signed value is undefined
      [-Wshift-negative-value]
        return posix_cpu_clock_get(THREAD_CLOCK, tp);
                                   ^~~~~~~~~~~~
kernel/time/posix-cpu-timers.c:1367:22: note: expanded from macro 'THREAD_CLOCK'
#define THREAD_CLOCK    MAKE_THREAD_CPUCLOCK(0, CPUCLOCK_SCHED)
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./include/linux/posix-timers.h:48:2: note: expanded from macro
'MAKE_THREAD_CPUCLOCK'
        MAKE_PROCESS_CPUCLOCK((tid), (clock) | CPUCLOCK_PERTHREAD_MASK)
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./include/linux/posix-timers.h:46:23: note: expanded from macro
'MAKE_PROCESS_CPUCLOCK'
        ((~(clockid_t) (pid) << 3) | (clockid_t) (clock))
          ~~~~~~~~~~~~~~~~~~ ^

If I understand C's operator precedence rules
(http://en.cppreference.com/w/c/language/operator_precedence)
correctly, then I suspect the problem is in the sub-expression:

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

where pid (an argument to the macro) is first cast to a clockid_t (aka
[signed] int), then negated, then shifted by 3 (oops, undefined
behavior).

Should the result after negation be cast to an unsigned int, or should
the left shift happen before negation?

CPUCLOCK_PID and CLOCKID_TO_FD seem to shift then negate, but
FD_TO_CLOCKID seems to have the same issue as MAKE_PROCESS_CPUCLOCK.

Changing the sub-expression to:

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

changes what it evaluates to.  Changing it to:

(~(unsigned) (pid) << 3)

or

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

or

(((unsigned) ~(clockid_t) (pid)) << 3) /* ugly */

does not.

I'm happy to send a patch with your suggestion.