signal: Extend exec_id to 64bits

commit d1e7fd6462ca9fc76650fbe6ca800e35b24267da upstream.

Replace the 32bit exec_id with a 64bit exec_id to make it impossible
to wrap the exec_id counter.  With care an attacker can cause exec_id
wrap and send arbitrary signals to a newly exec'd parent.  This
bypasses the signal sending checks if the parent changes their
credentials during exec.

The severity of this problem can been seen that in my limited testing
of a 32bit exec_id it can take as little as 19s to exec 65536 times.
Which means that it can take as little as 14 days to wrap a 32bit
exec_id.  Adam Zabrocki has succeeded wrapping the self_exe_id in 7
days.  Even my slower timing is in the uptime of a typical server.
Which means self_exec_id is simply a speed bump today, and if exec
gets noticably faster self_exec_id won't even be a speed bump.

Extending self_exec_id to 64bits introduces a problem on 32bit
architectures where reading self_exec_id is no longer atomic and can
take two read instructions.  Which means that is is possible to hit
a window where the read value of exec_id does not match the written
value.  So with very lucky timing after this change this still
remains expoiltable.

I have updated the update of exec_id on exec to use WRITE_ONCE
and the read of exec_id in do_notify_parent to use READ_ONCE
to make it clear that there is no locking between these two
locations.

Link: https://lore.kernel.org/kernel-hardening/20200324215049.GA3710@pi3.com.pl
Fixes: 2.3.23pre2
Cc: stable@vger.kernel.org
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/fs/exec.c b/fs/exec.c
index 74d88dab98dd58282f3e08b82ff9dd388f195846..ea16e3a6ae197a933eb378c8c69686b4611ee669 100644 (file)
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1382,7 +1382,7 @@ void setup_new_exec(struct linux_binprm * bprm)
 
        /* An exec changes our domain. We are no longer part of the thread
           group */
-       current->self_exec_id++;
+       WRITE_ONCE(current->self_exec_id, current->self_exec_id + 1);
        flush_signal_handlers(current, 0);
 }
 EXPORT_SYMBOL(setup_new_exec);

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 716ad1d8d95e1457dd1b42f51bebbf692e836830..cae52b0e9ff3844d3d8e765d4635158a92ae30a8 100644 (file)
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -939,8 +939,8 @@ struct task_struct {
        struct seccomp                  seccomp;
 
        /* Thread group tracking: */
-       u32                             parent_exec_id;
-       u32                             self_exec_id;
+       u64                             parent_exec_id;
+       u64                             self_exec_id;
 
        /* Protection against (de-)allocation: mm, files, fs, tty, keyrings, mems_allowed, mempolicy: */
        spinlock_t                      alloc_lock;

diff --git a/kernel/signal.c b/kernel/signal.c
index eea748174ade9f985d73d0e0024d636845a18123..7d3d35eb7a0ba5849cdd5e765f3e9f79432ed1c5 100644 (file)
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1931,7 +1931,7 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
                 * This is only possible if parent == real_parent.
                 * Check if it has changed security domain.
                 */
-               if (tsk->parent_exec_id != tsk->parent->self_exec_id)
+               if (tsk->parent_exec_id != READ_ONCE(tsk->parent->self_exec_id))
                        sig = SIGCHLD;
        }