Re: [RFC PATCH 0/3] mt-exec: fix deadlock with ptrace_attach()

[Oleg Nesterov <oleg@redhat.com>]

Hi Bernd,

On 11/10, Bernd Edlinger wrote:
>
> When the debugger wants to attach the de_thread the debug-user access rights are
> checked against the current user and additionally against the new user credentials.
> This I did by quickly switching the user credenitals to the next user and back again,
> under the cred_guard_mutex, which should make that safe.

Let me repeat, I can't really comment this part, I don't know if it is
actually safe. But the very fact your patch changes ->mm and ->cred of
the execing task in ptrace_attach() makes me worry... At least I think
you should update or remove this comment in begin_new_exec:

	/*
	 * cred_guard_mutex must be held at least to this point to prevent
	 * ptrace_attach() from altering our determination of the task's
	 * credentials; any time after this it may be unlocked.
	 */
	security_bprm_committed_creds(bprm);

> So at this time I have only one request for you.
> Could you please try out how the test case in my patch behaves with your fix?

The new TEST(attach2) added by your patch fails as expected, see 3/3.

   128  static long thread2_tid;
   129  static void *thread2(void *arg)
   130  {
   131          thread2_tid = syscall(__NR_gettid);
   132          sleep(2);
   133          execlp("false", "false", NULL);
   134          return NULL;
   135  }
   136
   137  TEST(attach2)
   138  {
   139          int s, k, pid = fork();
   140
   141          if (!pid) {
   142                  pthread_t pt;
   143
   144                  pthread_create(&pt, NULL, thread2, NULL);
   145                  pthread_join(pt, NULL);
   146                  return;
   147          }
   148
   149          sleep(1);
   150          k = ptrace(PTRACE_ATTACH, pid, 0L, 0L);
   151          ASSERT_EQ(k, 0);
   152          k = waitpid(-1, &s, 0);
   153          ASSERT_EQ(k, pid);
   154          ASSERT_EQ(WIFSTOPPED(s), 1);
   155          ASSERT_EQ(WSTOPSIG(s), SIGSTOP);
   156          k = ptrace(PTRACE_SETOPTIONS, pid, 0L, PTRACE_O_TRACEEXIT);
   157          ASSERT_EQ(k, 0);
   158          thread2_tid = ptrace(PTRACE_PEEKDATA, pid, &thread2_tid, 0L);
   159          ASSERT_NE(thread2_tid, -1);
   160          ASSERT_NE(thread2_tid, 0);
   161          ASSERT_NE(thread2_tid, pid);
   162          k = waitpid(-1, &s, WNOHANG);
   163          ASSERT_EQ(k, 0);
   164          sleep(2);
   165          /* deadlock may happen here */
   166          k = ptrace(PTRACE_ATTACH, thread2_tid, 0L, 0L);

PTRACE_ATTACH fails.

thread2() kills the old leader, takes it pid, execlp() succeeds.

Oleg.