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.
