linux-kernel - Re: [PATCH 1/1] signals: don't abuse __flush_signals() in selinux_bprm_committed

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: <10375684.YIDl5mrJKW@sifl>
Date:	Mon, 04 May 2015 15:43:28 -0400
From:	Paul Moore <paul@...l-moore.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...nel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Neil Brown <neilb@...e.de>, Evgeniy Polyakov <zbr@...emap.net>,
	Stephen Smalley <sds@...ho.nsa.gov>,
	Alex Williamson <alex.williamson@...hat.com>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	kvm <kvm@...r.kernel.org>, Eric Paris <eparis@...isplace.org>,
	selinux@...ho.nsa.gov
Subject: Re: [PATCH 1/1] signals: don't abuse __flush_signals() in selinux_bprm_committed_creds()

On Monday, May 04, 2015 06:45:58 PM Oleg Nesterov wrote:
> selinux_bprm_committed_creds()->__flush_signals() is not right, we
> shouldn't clear TIF_SIGPENDING unconditionally. There can be other
> reasons for signal_pending(): freezing(), JOBCTL_PENDING_MASK, and
> potentially more.
> 
> Also change this code to check fatal_signal_pending() rather than
> SIGNAL_GROUP_EXIT, it looks a bit better.
> 
> Now we can kill __flush_signals() before it finds another buggy user.

[NOTE: Added the SELinux list to the CC line]

This looks reasonable to me, I'm going to apply it to selinux#next today.

> Note: this code looks racy, we can flush a signal which was sent after
> the task SID has been updated.

The whole signal flush thread has started some discussions about how we are 
currently handling this, and if it still makes sense.  Like many things, it 
seemed like a good idea at the time, but after several years we're debating if 
that is still the case.  I expect we'll be changing this code soon.

> Signed-off-by: Oleg Nesterov <oleg@...hat.com>
> ---
>  include/linux/sched.h    |    1 -
>  kernel/signal.c          |   13 ++++---------
>  security/selinux/hooks.c |    6 ++++--
>  3 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 8db31ef..eb1ac84 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2345,7 +2345,6 @@ extern void sched_dead(struct task_struct *p);
> 
>  extern void proc_caches_init(void);
>  extern void flush_signals(struct task_struct *);
> -extern void __flush_signals(struct task_struct *);
>  extern void ignore_signals(struct task_struct *);
>  extern void flush_signal_handlers(struct task_struct *, int force_default);
> extern int dequeue_signal(struct task_struct *tsk, sigset_t *mask,
> siginfo_t *info); diff --git a/kernel/signal.c b/kernel/signal.c
> index 16a3052..837ca7d 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -414,21 +414,16 @@ void flush_sigqueue(struct sigpending *queue)
>  }
> 
>  /*
> - * Flush all pending signals for a task.
> + * Flush all pending signals for this kthread.
>   */
> -void __flush_signals(struct task_struct *t)
> -{
> -	clear_tsk_thread_flag(t, TIF_SIGPENDING);
> -	flush_sigqueue(&t->pending);
> -	flush_sigqueue(&t->signal->shared_pending);
> -}
> -
>  void flush_signals(struct task_struct *t)
>  {
>  	unsigned long flags;
> 
>  	spin_lock_irqsave(&t->sighand->siglock, flags);
> -	__flush_signals(t);
> +	clear_tsk_thread_flag(t, TIF_SIGPENDING);
> +	flush_sigqueue(&t->pending);
> +	flush_sigqueue(&t->signal->shared_pending);
>  	spin_unlock_irqrestore(&t->sighand->siglock, flags);
>  }
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 6da7532..6907d11 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2397,10 +2397,12 @@ static void selinux_bprm_committed_creds(struct
> linux_binprm *bprm) for (i = 0; i < 3; i++)
>  			do_setitimer(i, &itimer, NULL);
>  		spin_lock_irq(&current->sighand->siglock);
> -		if (!(current->signal->flags & SIGNAL_GROUP_EXIT)) {
> -			__flush_signals(current);
> +		if (!fatal_signal_pending(current)) {
> +			flush_sigqueue(&current->pending);
> +			flush_sigqueue(&current->signal->shared_pending);
>  			flush_signal_handlers(current, 1);
>  			sigemptyset(&current->blocked);
> +			recalc_sigpending();
>  		}
>  		spin_unlock_irq(&current->sighand->siglock);
>  	}

-- 
paul moore
www.paul-moore.com

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/