linux-kernel - Re: [patch v4 04/27] posix-timers: Cure si_sys

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87r095yz28.fsf@email.froward.int.ebiederm.org>
Date: Fri, 27 Sep 2024 09:02:39 -0500
From: "Eric W. Biederman" <ebiederm@...ssion.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: LKML <linux-kernel@...r.kernel.org>,  Anna-Maria Behnsen
 <anna-maria@...utronix.de>,  Frederic Weisbecker <frederic@...nel.org>,
  John Stultz <jstultz@...gle.com>,  Peter Zijlstra <peterz@...radead.org>,
  Ingo Molnar <mingo@...nel.org>,  Stephen Boyd <sboyd@...nel.org>,  Oleg
 Nesterov <oleg@...hat.com>
Subject: Re: [patch v4 04/27] posix-timers: Cure si_sys_private race

Thomas Gleixner <tglx@...utronix.de> writes:

> From: Thomas Gleixner <tglx@...utronix.de>
>
> The si_sys_private member of the siginfo which is embedded in the
> preallocated sigqueue is used by the posix timer code to decide whether a
> timer must be reprogrammed on signal delivery.
>
> The handling of this is racy as a long standing comment in that code
> documents. It is modified with the timer lock held, but without sighand
> lock being held. The actual signal delivery code checks for it under
> sighand lock without holding the timer lock.

I suspect this falls under the ancient all integers are atomic rule
in practice.

> Hand the new value to send_sigqueue() as argument and store it with sighand
> lock held.

Is there any way we can simply remove the hack of using si_sys_private,
and use a field in the structure that contains the preallocated signal?

I don't have any issues with updating send_siqueue so that the locking
is consistent.  However can we possibly name the argument something like
it_requeue_pending instead of si_private?

Eric

> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@...radead.org>
>
> ---
>  include/linux/sched/signal.h |  2 +-
>  kernel/signal.c              | 10 +++++++++-
>  kernel/time/posix-timers.c   | 15 +--------------
>  3 files changed, 11 insertions(+), 16 deletions(-)
> ---
> diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
> index c8ed09ac29ac..bd9f569231d9 100644
> --- a/include/linux/sched/signal.h
> +++ b/include/linux/sched/signal.h
> @@ -340,7 +340,7 @@ extern int send_sig(int, struct task_struct *, int);
>  extern int zap_other_threads(struct task_struct *p);
>  extern struct sigqueue *sigqueue_alloc(void);
>  extern void sigqueue_free(struct sigqueue *);
> -extern int send_sigqueue(struct sigqueue *, struct pid *, enum pid_type);
> +extern int send_sigqueue(struct sigqueue *, struct pid *, enum pid_type, int si_private);
>  extern int do_sigaction(int, struct k_sigaction *, struct k_sigaction *);
>  
>  static inline void clear_notify_signal(void)
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 3d2e087283ab..443baadb5ab0 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1915,7 +1915,7 @@ void sigqueue_free(struct sigqueue *q)
>  		__sigqueue_free(q);
>  }
>  
> -int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
> +int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type, int si_private)
>  {
>  	int sig = q->info.si_signo;
>  	struct sigpending *pending;
> @@ -1950,6 +1950,14 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
>  	if (!likely(lock_task_sighand(t, &flags)))
>  		goto ret;
>  
> +	/*
> +	 * Update @q::info::si_sys_private for posix timer signals with
> +	 * sighand locked to prevent a race against dequeue_signal() which
> +	 * decides based on si_sys_private whether to invoke
> +	 * posixtimer_rearm() or not.
> +	 */
> +	q->info.si_sys_private = si_private;
> +
>  	ret = 1; /* the signal is ignored */
>  	result = TRACE_SIGNAL_IGNORED;
>  	if (!prepare_signal(sig, t, false))
> diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
> index bcd5e56412e7..b6cca1ed2f90 100644
> --- a/kernel/time/posix-timers.c
> +++ b/kernel/time/posix-timers.c
> @@ -299,21 +299,8 @@ int posix_timer_queue_signal(struct k_itimer *timr)
>  	if (timr->it_interval)
>  		si_private = ++timr->it_requeue_pending;
>  
> -	/*
> -	 * FIXME: if ->sigq is queued we can race with
> -	 * dequeue_signal()->posixtimer_rearm().
> -	 *
> -	 * If dequeue_signal() sees the "right" value of
> -	 * si_sys_private it calls posixtimer_rearm().
> -	 * We re-queue ->sigq and drop ->it_lock().
> -	 * posixtimer_rearm() locks the timer
> -	 * and re-schedules it while ->sigq is pending.
> -	 * Not really bad, but not that we want.
> -	 */
> -	timr->sigq->info.si_sys_private = si_private;
> -
>  	type = !(timr->it_sigev_notify & SIGEV_THREAD_ID) ? PIDTYPE_TGID : PIDTYPE_PID;
> -	ret = send_sigqueue(timr->sigq, timr->it_pid, type);
> +	ret = send_sigqueue(timr->sigq, timr->it_pid, type, si_private);
>  	/* If we failed to send the signal the timer stops. */
>  	return ret > 0;
>  }