[PATCH v2] Relax si_code check in rt_sigqueueinfo and rt_tgsigqueueinfo

[PATCH v2] Relax si_code check in rt_sigqueueinfo and rt_tgsigqueueinfo

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

Commit da48524eb206 ("Prevent rt_sigqueueinfo and rt_tgsigqueueinfo
from spoofing the signal code") made the check on si_code too strict.
There are several legitimate places where glibc wants to queue a
negative si_code different from SI_QUEUE:

 - This was first noticed with glibc's aio implementation, which wants
   to queue a signal with si_code SI_ASYNCIO; the current kernel
   causes glibc's tst-aio4 test to fail because rt_sigqueueinfo()
   fails with EPERM.

 - Further examination of the glibc source shows that getaddrinfo_a()
   wants to use SI_ASYNCNL (which the kernel does not even define).
   The timer_create() fallback code wants to queue signals with SI_TIMER.

As suggested by Oleg Nesterov <oleg@redhat.com>, loosen the check to
forbid only the problematic SI_TKILL case.

Reported-by: Klaus Dittrich <kladit@arcor.de>
Cc: <stable@kernel.org>
Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 kernel/signal.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 324eff5..b2bfa3a 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2437,7 +2437,7 @@ SYSCALL_DEFINE3(rt_sigqueueinfo, pid_t, pid, int, sig,
 	/* Not even root can pretend to send signals from the kernel.
 	 * Nor can they impersonate a kill()/tgkill(), which adds source info.
 	 */
-	if (info.si_code != SI_QUEUE) {
+	if (info.si_code >= 0 || info.si_code == SI_TKILL) {
 		/* We used to allow any < 0 si_code */
 		WARN_ON_ONCE(info.si_code < 0);
 		return -EPERM;
@@ -2457,7 +2457,7 @@ long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig, siginfo_t *info)
 	/* Not even root can pretend to send signals from the kernel.
 	 * Nor can they impersonate a kill()/tgkill(), which adds source info.
 	 */
-	if (info->si_code != SI_QUEUE) {
+	if (info->si_code >= 0 || info->si_code == SI_TKILL) {
 		/* We used to allow any < 0 si_code */
 		WARN_ON_ONCE(info->si_code < 0);
 		return -EPERM;

Re: [PATCH v2] Relax si_code check in rt_sigqueueinfo and rt_tgsigqueueinfo

[Julien Tinnes]

Ohh, good catch!

It's not ideal that we have to relax this check, but as long as kernel
codes, SI_USER and SI_TKILL are not spoofable, I think we're in ok
shape.

Thanks,

Julien

On Mon, Mar 28, 2011 at 2:13 PM, Roland Dreier <roland@kernel.org> wrote:
> From: Roland Dreier <roland@purestorage.com>
>
> Commit da48524eb206 ("Prevent rt_sigqueueinfo and rt_tgsigqueueinfo
> from spoofing the signal code") made the check on si_code too strict.
> There are several legitimate places where glibc wants to queue a
> negative si_code different from SI_QUEUE:
>
>  - This was first noticed with glibc's aio implementation, which wants
>   to queue a signal with si_code SI_ASYNCIO; the current kernel
>   causes glibc's tst-aio4 test to fail because rt_sigqueueinfo()
>   fails with EPERM.
>
>  - Further examination of the glibc source shows that getaddrinfo_a()
>   wants to use SI_ASYNCNL (which the kernel does not even define).
>   The timer_create() fallback code wants to queue signals with SI_TIMER.
>
> As suggested by Oleg Nesterov <oleg@redhat.com>, loosen the check to
> forbid only the problematic SI_TKILL case.
>
> Reported-by: Klaus Dittrich <kladit@arcor.de>
> Cc: <stable@kernel.org>
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---
>  kernel/signal.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 324eff5..b2bfa3a 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2437,7 +2437,7 @@ SYSCALL_DEFINE3(rt_sigqueueinfo, pid_t, pid, int, sig,
>        /* Not even root can pretend to send signals from the kernel.
>         * Nor can they impersonate a kill()/tgkill(), which adds source info.
>         */
> -       if (info.si_code != SI_QUEUE) {
> +       if (info.si_code >= 0 || info.si_code == SI_TKILL) {
>                /* We used to allow any < 0 si_code */
>                WARN_ON_ONCE(info.si_code < 0);
>                return -EPERM;
> @@ -2457,7 +2457,7 @@ long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig, siginfo_t *info)
>        /* Not even root can pretend to send signals from the kernel.
>         * Nor can they impersonate a kill()/tgkill(), which adds source info.
>         */
> -       if (info->si_code != SI_QUEUE) {
> +       if (info->si_code >= 0 || info->si_code == SI_TKILL) {
>                /* We used to allow any < 0 si_code */
>                WARN_ON_ONCE(info->si_code < 0);
>                return -EPERM;
>

Re: [PATCH v2] Relax si_code check in rt_sigqueueinfo and rt_tgsigqueueinfo

[Oleg Nesterov]

On 03/28, Roland Dreier wrote:
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 324eff5..b2bfa3a 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2437,7 +2437,7 @@ SYSCALL_DEFINE3(rt_sigqueueinfo, pid_t, pid, int, sig,
>  	/* Not even root can pretend to send signals from the kernel.
>  	 * Nor can they impersonate a kill()/tgkill(), which adds source info.
>  	 */
> -	if (info.si_code != SI_QUEUE) {
> +	if (info.si_code >= 0 || info.si_code == SI_TKILL) {
>  		/* We used to allow any < 0 si_code */
>  		WARN_ON_ONCE(info.si_code < 0);

This is equivalent to WARN_ON_ONCE(SI_TKILL)... doesn't matter, please ignore.

Thanks, I think -stable needs this patch asap. It turns out 2.6.32.36, 2.6.33.9,
2.6.37.6 and 2.6.38.2 pulled da48524eb20662618854bb3df2db01fc65f3070c.

If this change breaks something too, we can make even more conservative check.

Acked-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH v2] Relax si_code check in rt_sigqueueinfo and rt_tgsigqueueinfo

[Roland Dreier]

On Tue, Mar 29, 2011 at 5:19 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> Thanks, I think -stable needs this patch asap. It turns out 2.6.32.36, 2.6.33.9,
> 2.6.37.6 and 2.6.38.2 pulled da48524eb20662618854bb3df2db01fc65f3070c.

Agree... the commit in Linus's tree is tagged for stable@.

The only issue I think is that Greg said he's not doing any more
2.6.37 releases?

Greg, this bug breaks the glibc test suite so maybe it's worth doing one more
release?

Re: [PATCH v2] Relax si_code check in rt_sigqueueinfo and rt_tgsigqueueinfo

[Greg KH]

On Tue, Mar 29, 2011 at 10:26:55AM -0700, Roland Dreier wrote:
> On Tue, Mar 29, 2011 at 5:19 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > Thanks, I think -stable needs this patch asap. It turns out 2.6.32.36, 2.6.33.9,
> > 2.6.37.6 and 2.6.38.2 pulled da48524eb20662618854bb3df2db01fc65f3070c.
> 
> Agree... the commit in Linus's tree is tagged for stable@.
> 
> The only issue I think is that Greg said he's not doing any more
> 2.6.37 releases?
> 
> Greg, this bug breaks the glibc test suite so maybe it's worth doing one more
> release?

Hm, I just checked the distros that I know of that are using the .37
kernel and they already have this patch in them, so it's not really
worth it to me to do another spin of the .37-stable tree.  Especially as
I'd like to have everyone move on to .38-stable instead.

thanks,

greg k-h