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.