stable.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kyle Huey <me@kylehuey.com>
To: gregkh@linuxfoundation.org, "Robert O'Callahan" <robert@ocallahan.org>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>,
	Kees Cook <keescook@chromium.org>, Kyle Huey <khuey@kylehuey.com>,
	Oliver Sang <oliver.sang@intel.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	stable@vger.kernel.org
Subject: Re: FAILED: patch "[PATCH] signal: Don't always set SA_IMMUTABLE for forced signals" failed to apply to 5.15-stable tree
Date: Tue, 23 Nov 2021 17:33:19 -0800	[thread overview]
Message-ID: <CAP045ApHdVjC59KE7+morWY_5j4px3O0Fm6F6-cuJ+p6Q9PCPA@mail.gmail.com> (raw)
In-Reply-To: <163758427225348@kroah.com>

On Mon, Nov 22, 2021 at 4:31 AM <gregkh@linuxfoundation.org> wrote:
>
>
> The patch below does not apply to the 5.15-stable tree.
> If someone wants it applied there, or to any other stable or longterm
> tree, then please email the backport, including the original git commit
> id to <stable@vger.kernel.org>.
>
> thanks,
>
> greg k-h
>
> ------------------ original commit in Linus's tree ------------------
>
> From e349d945fac76bddc78ae1cb92a0145b427a87ce Mon Sep 17 00:00:00 2001
> From: "Eric W. Biederman" <ebiederm@xmission.com>
> Date: Thu, 18 Nov 2021 11:11:13 -0600
> Subject: [PATCH] signal: Don't always set SA_IMMUTABLE for forced signals
>
> Recently to prevent issues with SECCOMP_RET_KILL and similar signals
> being changed before they are delivered SA_IMMUTABLE was added.
>
> Unfortunately this broke debuggers[1][2] which reasonably expect to be
> able to trap synchronous SIGTRAP and SIGSEGV even when the target
> process is not configured to handle those signals.
>
> Update force_sig_to_task to support both the case when we can allow
> the debugger to intercept and possibly ignore the signal and the case
> when it is not safe to let userspace know about the signal until the
> process has exited.
>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Reported-by: Kyle Huey <me@kylehuey.com>
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Cc: stable@vger.kernel.org
> [1] https://lkml.kernel.org/r/CAP045AoMY4xf8aC_4QU_-j7obuEPYgTcnQQP3Yxk=2X90jtpjw@mail.gmail.com
> [2] https://lkml.kernel.org/r/20211117150258.GB5403@xsang-OptiPlex-9020
> Fixes: 00b06da29cf9 ("signal: Add SA_IMMUTABLE to ensure forced siganls do not get changed")
> Link: https://lkml.kernel.org/r/877dd5qfw5.fsf_-_@email.froward.int.ebiederm.org
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Tested-by: Kees Cook <keescook@chromium.org>
> Tested-by: Kyle Huey <khuey@kylehuey.com>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 7c4b7ae714d4..7815e1bbeddc 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1298,6 +1298,12 @@ int do_send_sig_info(int sig, struct kernel_siginfo *info, struct task_struct *p
>         return ret;
>  }
>
> +enum sig_handler {
> +       HANDLER_CURRENT, /* If reachable use the current handler */
> +       HANDLER_SIG_DFL, /* Always use SIG_DFL handler semantics */
> +       HANDLER_EXIT,    /* Only visible as the process exit code */
> +};
> +
>  /*
>   * Force a signal that the process can't ignore: if necessary
>   * we unblock the signal and change any SIG_IGN to SIG_DFL.
> @@ -1310,7 +1316,8 @@ int do_send_sig_info(int sig, struct kernel_siginfo *info, struct task_struct *p
>   * that is why we also clear SIGNAL_UNKILLABLE.
>   */
>  static int
> -force_sig_info_to_task(struct kernel_siginfo *info, struct task_struct *t, bool sigdfl)
> +force_sig_info_to_task(struct kernel_siginfo *info, struct task_struct *t,
> +       enum sig_handler handler)
>  {
>         unsigned long int flags;
>         int ret, blocked, ignored;
> @@ -1321,9 +1328,10 @@ force_sig_info_to_task(struct kernel_siginfo *info, struct task_struct *t, bool
>         action = &t->sighand->action[sig-1];
>         ignored = action->sa.sa_handler == SIG_IGN;
>         blocked = sigismember(&t->blocked, sig);
> -       if (blocked || ignored || sigdfl) {
> +       if (blocked || ignored || (handler != HANDLER_CURRENT)) {
>                 action->sa.sa_handler = SIG_DFL;
> -               action->sa.sa_flags |= SA_IMMUTABLE;
> +               if (handler == HANDLER_EXIT)
> +                       action->sa.sa_flags |= SA_IMMUTABLE;
>                 if (blocked) {
>                         sigdelset(&t->blocked, sig);
>                         recalc_sigpending_and_wake(t);
> @@ -1343,7 +1351,7 @@ force_sig_info_to_task(struct kernel_siginfo *info, struct task_struct *t, bool
>
>  int force_sig_info(struct kernel_siginfo *info)
>  {
> -       return force_sig_info_to_task(info, current, false);
> +       return force_sig_info_to_task(info, current, HANDLER_CURRENT);
>  }
>
>  /*
> @@ -1660,7 +1668,7 @@ void force_fatal_sig(int sig)
>         info.si_code = SI_KERNEL;
>         info.si_pid = 0;
>         info.si_uid = 0;
> -       force_sig_info_to_task(&info, current, true);
> +       force_sig_info_to_task(&info, current, HANDLER_SIG_DFL);
>  }
>
>  /*
> @@ -1693,7 +1701,7 @@ int force_sig_fault_to_task(int sig, int code, void __user *addr
>         info.si_flags = flags;
>         info.si_isr = isr;
>  #endif
> -       return force_sig_info_to_task(&info, t, false);
> +       return force_sig_info_to_task(&info, t, HANDLER_CURRENT);
>  }
>
>  int force_sig_fault(int sig, int code, void __user *addr
> @@ -1813,7 +1821,8 @@ int force_sig_seccomp(int syscall, int reason, bool force_coredump)
>         info.si_errno = reason;
>         info.si_arch = syscall_get_arch(current);
>         info.si_syscall = syscall;
> -       return force_sig_info_to_task(&info, current, force_coredump);
> +       return force_sig_info_to_task(&info, current,
> +               force_coredump ? HANDLER_EXIT : HANDLER_CURRENT);
>  }
>
>  /* For the crazy architectures that include trap information in
>

Since this is taken care of now, AFAICT, I do have one additional
question. I reported the regression to LKML a day or so before 5.15.3
was cut. What should I have noticed to see that the regressing
changeset was going to 5.15 and where should I have said "hey please
don't ship this on 5.15 yet"?

I'd like to know what to do next time :)

- Kyle

  parent reply	other threads:[~2021-11-24  1:33 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-22 12:31 gregkh
2021-11-23 15:08 ` Kyle Huey
2021-11-23 17:29 ` Thomas Backlund
2021-11-23 18:12   ` Greg KH
2021-11-23 19:24     ` Eric W. Biederman
2021-11-23 20:00       ` Eric W. Biederman
2021-11-23 20:00       ` Thomas Backlund
2021-11-23 21:41         ` Eric W. Biederman
2021-11-23 21:56           ` Thomas Backlund
2021-11-24  1:33 ` Kyle Huey [this message]
2021-11-24  5:15   ` Eric W. Biederman
2021-11-29  6:23     ` Kyle Huey
2021-11-29  6:43       ` Greg KH

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAP045ApHdVjC59KE7+morWY_5j4px3O0Fm6F6-cuJ+p6Q9PCPA@mail.gmail.com \
    --to=me@kylehuey.com \
    --cc=ebiederm@xmission.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=keescook@chromium.org \
    --cc=khuey@kylehuey.com \
    --cc=oliver.sang@intel.com \
    --cc=robert@ocallahan.org \
    --cc=stable@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    --subject='Re: FAILED: patch "[PATCH] signal: Don'\''t always set SA_IMMUTABLE for forced signals" failed to apply to 5.15-stable tree' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).