user namespace: make signal.c respect user namespaces

Add to the dev_state and alloc_async structures the user namespace corresponding to the uid and euid. Pass these to kill_pid_info_as_uid(), which can then implement a proper, user-namespace-aware uid check.

Quoted 4 lines · Serge E. Hallyn

Add to the dev_state and alloc_async structures the user namespace corresponding to the uid and euid. Pass these to kill_pid_info_as_uid(), which can then implement a proper, user-namespace-aware uid check.

IOW, we add the additional "user_namespace *" member/argument, and use it along with uid/euid.

I am not really sure, but can't we simplify this?

drivers/usb/core/devio.c struct dev_state { · line 73

Quoted 6 lines

drivers/usb/core/devio.c +1 -0

@@ -68,6 +69,7 @@ struct dev_state {
 	wait_queue_head_t wait;     /* wake up if a request completed */
 	unsigned int discsignr;
 	struct pid *disc_pid;
+	struct user_namespace *user_ns;
 	uid_t disc_uid, disc_euid;

Can't we add "const struct cred *disc_cred" and kill disc_uid/disc_euid instead?

Then we redefine kill_pid_info_as_uid() as kill_pid_info_as_cred(...cred...), it can use cred->cred->uid/euid directly.

devio.c does get_cred/put_cred instead of get_user_ns/put_user_ns.

What do you think?

Add to the dev_state and alloc_async structures the user namespace corresponding to the uid and euid. Pass these to kill_pid_info_as_uid(), which can then implement a proper, user-namespace-aware uid check.

Quoted 4 lines · Serge E. Hallyn

Add to the dev_state and alloc_async structures the user namespace corresponding to the uid and euid. Pass these to kill_pid_info_as_uid(), which can then implement a proper, user-namespace-aware uid check.

Looks correct.

But I have off-topic question. And in fact I am a bit confused, please help.

First of all, I assume that CLONE_NEWUSER is the only way to change ->user_ns, right?

And, looking at copy_creds() I think that cred->user_ns is always equal to cred->user->user_ns. However, grep shows a lot of cred->user->user_ns examples. Why?

kernel/signal.c int kill_proc_info(int sig, struct siginfo *info, pid_t pid) · line 1363

Quoted 6 lines

kernel/signal.c +6 -0

+static int kill_as_cred_perm(const struct cred *cred,
+			     struct task_struct *target)
+{
+	const struct cred *pcred = __task_cred(target);
+	if (cred->user_ns != pcred->user_ns)
+		return 0;

Should we really fail if cred->user_ns == pcred->user_ns->creator ? (or creator of creator, etc).

IOW, shouldn't this match kill_ok_by_cred() path which (at least cap_capable) checks the ->creator chain when ->user_ns differ?

Drat. Greg, sorry about not Cc:ing you on the original patch. Please let me know if you'd like me to resend to you.

(re-sending to Cc: Greg and linux-usb)

Add to the dev_state and alloc_async structures the user namespace corresponding to the uid and euid. Pass these to kill_pid_info_as_uid(), which can then implement a proper, user-namespace-aware uid check.

Changelog:
Sep 20: Per Oleg's suggestion: Instead of caching and passing user namespace,
uid, and euid each separately, pass a struct cred.

drivers/usb/core/devio.c +2 -4

@@ -393,9 +395,8 @@ static void async_completed(struct urb *urb)
 	struct dev_state *ps = as->ps;
 	struct siginfo sinfo;
 	struct pid *pid = NULL;
-	uid_t uid = 0;
-	uid_t euid = 0;
 	u32 secid = 0;
+	const struct cred *cred = NULL;
 	int signr;
 
 	spin_lock(&ps->lock);

@@ -408,8 +409,7 @@ static void async_completed(struct urb *urb)
 		sinfo.si_code = SI_ASYNCIO;
 		sinfo.si_addr = as->userurb;
 		pid = as->pid;
-		uid = as->uid;
-		euid = as->euid;
+		cred = as->cred;
 		secid = as->secid;
 	}
 	snoop(&urb->dev->dev, "urb complete\n");

Quoted 6 lines · Serge E. Hallyn

(re-sending to Cc: Greg and linux-usb)

Add to the dev_state and alloc_async structures the user namespace corresponding to the uid and euid. Pass these to kill_pid_info_as_uid(), which can then implement a proper, user-namespace-aware uid check.

Quoted 3 lines

Changelog:
Sep 20: Per Oleg's suggestion: Instead of caching and passing user namespace,
uid, and euid each separately, pass a struct cred.

This should be broken up into two separate patches: One to add kill_pid_info_as_cred() and the other to modify the usbfs driver.

Quoted 27 lines

drivers/usb/core/devio.c +2 -4

@@ -393,9 +395,8 @@ static void async_compled ted(struct urb *urb)
 	struct dev_state *ps = as->ps;
 	struct siginfo sinfo;
 	struct pid *pid = NULL;
-	uid_t uid = 0;
-	uid_t euid = 0;
 	u32 secid = 0;
+	const struct cred *cred = NULL;
 	int signr;
 
 	spin_lock(&ps->lock);

@@ -408,8 +409,7 @@ static void async_completed(struct urb *urb)
 		sinfo.si_code = SI_ASYNCIO;
 		sinfo.si_addr = as->userurb;
 		pid = as->pid;
-		uid = as->uid;
-		euid = as->euid;
+		cred = as->cred;
 		secid = as->secid;
 	}
 	snoop(&urb->dev->dev, "urb complete\n");

@@ -423,8 +423,7 @@ static void async_completed(struct urb *urb)
 	spin_unlock(&ps->lock);
 

drivers/usb/core/devio.c static void async_completed(struct urb *urb) · line 426

Quoted 4 lines

drivers/usb/core/devio.c +1 -2

 	if (signr)
-		kill_pid_info_as_uid(sinfo.si_signo, &sinfo, pid, uid,
-				      euid, secid);
+		kill_pid_info_as_cred(sinfo.si_signo, &sinfo, pid, cred, secid);

This continues a bug that already exists in the current code. Once ps->lock is released, there is no guarantee that the async structure will still exist. It may already have been freed, and the reference to

as->cred may already have been dropped. That's why the local copies have to be made above. cred shouldn't be a simple copy of as->cred; it should also increment the reference count.

drivers/usb/core/devio.c static int usbdev_open(struct inode *inode, struct file *file) · line 708

Quoted 7 lines

drivers/usb/core/devio.c +1 -2

@@ -706,8 +705,7 @@ static int usbdev_open(struct inode *inode, struct file *file)
 	init_waitqueue_head(&ps->wait);
 	ps->discsignr = 0;
 	ps->disc_pid = get_pid(task_pid(current));
-	ps->disc_uid = cred->uid;
-	ps->disc_euid = cred->euid;
+	ps->cred = get_cred(cred);

You might as well get rid of the "cred" local variable. It isn't used for anything except this assignment.

Alan Stern

Quoted 12 lines · Alan Stern

Quoted 9 lines · Serge E. Hallyn

(re-sending to Cc: Greg and linux-usb)

Add to the dev_state and alloc_async structures the user namespace corresponding to the uid and euid. Pass these to kill_pid_info_as_uid(), which can then implement a proper, user-namespace-aware uid check.

Changelog:
Sep 20: Per Oleg's suggestion: Instead of caching and passing user namespace,
uid, and euid each separately, pass a struct cred.

Quoted 2 lines

This should be broken up into two separate patches: One to add kill_pid_info_as_cred() and the other to modify the usbfs driver.

It seems like that would make the first patch harder to review (since it won't just show the changes from kill_pid_info_as_uid to kill_pid_info_as_cred), but I'll go ahead and split it up. I assume kill_pid_info_as_uid should be removed in a third patch?

Quoted 8 lines

drivers/usb/core/devio.c static void async_completed(struct urb *urb) · line 426

Quoted 4 lines

drivers/usb/core/devio.c +1 -2

 	if (signr)
-		kill_pid_info_as_uid(sinfo.si_signo, &sinfo, pid, uid,
-				      euid, secid);
+		kill_pid_info_as_cred(sinfo.si_signo, &sinfo, pid, cred, secid);

This continues a bug that already exists in the current code. Once ps->lock is released, there is no guarantee that the async structure will still exist. It may already have been freed, and the reference to

Yikes. That makes sense. I'll fix that for the cred and the pid as well then?

Quoted 15 lines

as->cred may already have been dropped. That's why the local copies have to be made above. cred shouldn't be a simple copy of as->cred; it should also increment the reference count.

drivers/usb/core/devio.c static int usbdev_open(struct inode *inode, struct file *file) · line 708

Quoted 7 lines

drivers/usb/core/devio.c +1 -2

@@ -706,8 +705,7 @@ static int usbdev_open(struct inode *inode, struct file *file)
 	init_waitqueue_head(&ps->wait);
 	ps->discsignr = 0;
 	ps->disc_pid = get_pid(task_pid(current));
-	ps->disc_uid = cred->uid;
-	ps->disc_euid = cred->euid;
+	ps->cred = get_cred(cred);

You might as well get rid of the "cred" local variable. It isn't used for anything except this assignment.

Quoted 1 line

Alan Stern

Thanks for looking, Alan.

Quoted 16 lines · Serge E. Hallyn

Quoted 14 lines · Alan Stern

Quoted 9 lines · Serge E. Hallyn

(re-sending to Cc: Greg and linux-usb)

Add to the dev_state and alloc_async structures the user namespace corresponding to the uid and euid. Pass these to kill_pid_info_as_uid(), which can then implement a proper, user-namespace-aware uid check.

Changelog:
Sep 20: Per Oleg's suggestion: Instead of caching and passing user namespace,
uid, and euid each separately, pass a struct cred.

This should be broken up into two separate patches: One to add kill_pid_info_as_cred() and the other to modify the usbfs driver.

Quoted 4 lines

It seems like that would make the first patch harder to review (since it won't just show the changes from kill_pid_info_as_uid to kill_pid_info_as_cred), but I'll go ahead and split it up. I assume kill_pid_info_as_uid should be removed in a third patch?

No, what I meant was that all the changes to /kernel/signal.c should be in one patch and all the changes to drivers/usb/core/devio.c should be in a second patch.

You did check to make sure there are no other references to kill_pid_info_as_uid() in the kernel?

Quoted 9 lines

Quoted 8 lines

drivers/usb/core/devio.c static void async_completed(struct urb *urb) · line 426

Quoted 4 lines

drivers/usb/core/devio.c +1 -2

 	if (signr)
-		kill_pid_info_as_uid(sinfo.si_signo, &sinfo, pid, uid,
-				      euid, secid);
+		kill_pid_info_as_cred(sinfo.si_signo, &sinfo, pid, cred, secid);

This continues a bug that already exists in the current code. Once ps->lock is released, there is no guarantee that the async structure will still exist. It may already have been freed, and the reference to

Quoted 2 lines

Yikes. That makes sense. I'll fix that for the cred and the pid as well then?

Right.

Quoted 8 lines

Quoted 7 lines

Quoted 2 lines

This should be broken up into two separate patches: One to add kill_pid_info_as_cred() and the other to modify the usbfs driver.

It seems like that would make the first patch harder to review (since it won't just show the changes from kill_pid_info_as_uid to kill_pid_info_as_cred), but I'll go ahead and split it up. I assume kill_pid_info_as_uid should be removed in a third patch?

Quoted 3 lines

No, what I meant was that all the changes to /kernel/signal.c should be in one patch and all the changes to drivers/usb/core/devio.c should be in a second patch.

But doing that in two patches would not be bisect-safe.

__send_signal: convert the uid being sent in SI_USER to the target task's user namespace.

do_notify_parent and do_notify_parent_cldstop: map task's uid to parent's
user namespace

ptrace_signal: map parent's uid into current's user namespace before including in signal to current.

Quoted 9 lines · Serge E. Hallyn

__send_signal: convert the uid being sent in SI_USER to the target task's user namespace.

do_notify_parent and do_notify_parent_cldstop: map task's uid to parent's
user namespace

ptrace_signal: map parent's uid into current's user namespace before including in signal to current.

And all of them follow the same pattern,

kernel/signal.c static int __send_signal(int sig, struct siginfo *info, struct task_struct *t, · line 1078

Quoted 7 lines

kernel/signal.c +2 -1

@@ -1073,7 +1074,8 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 			q->info.si_code = SI_USER;
 			q->info.si_pid = task_tgid_nr_ns(current,
 							task_active_pid_ns(t));
-			q->info.si_uid = current_uid();
+			q->info.si_uid = user_ns_map_uid(task_cred_xxx(t, user_ns),
+					current_cred(), current_uid());

Up to you, but may be we can add a helper? Something like

	static inline uid_t good_name(struct task_struct *from, struct task_struct *to)
	{
		// the caller does rcu_read_lock() if needed
		const struct cred *from_cred = __task_cred(from);
		return user_ns_map_uid(task_cred_xxx(to, user_ns),
					from_cred, from_cred->uid);
	}

As for __send_signal() in particular, I guess we could do

		q->info.si_uid = from_ancestor_ns ? 0 : current_uid();

instead, but otoh perhaps it is better to use user_ns_map_uid() anyway for consistency.

Quoted 6 lines

kernel/signal.c +1 -0

@@ -2118,11 +2124,16 @@ static int ptrace_signal(int signr, siginfo_t *info,
 	 * have updated *info via PTRACE_SETSIGINFO.
 	 */
 	if (signr != info->si_signo) {
+		const struct cred *pcred;
 		info->si_signo = signr;

kernel/signal.c static int ptrace_signal(int signr, siginfo_t *info, · line 2132

Quoted 5 lines

kernel/signal.c +1 -1

 		info->si_errno = 0;
 		info->si_code = SI_USER;
 		info->si_pid = task_pid_vnr(current->parent);
-		info->si_uid = task_uid(current->parent);
+		rcu_read_lock();

In this case please add rcu_read_lock() earlier, before task_pid_vnr(). It has the same (theoretical) reasons for rcu lock.

Quoted 8 lines · Oleg Nesterov

	static inline uid_t good_name(struct task_struct *from, struct task_struct *to)
	{
		// the caller does rcu_read_lock() if needed
		const struct cred *from_cred = __task_cred(from);
		return user_ns_map_uid(task_cred_xxx(to, user_ns),
					from_cred, from_cred->uid);
	}

That looks great, thanks. I couldn't think it up myself, but now that I see it in your email, I see this would be very valuable in helping make this code more readable :)

Quoted 11 lines · Serge E. Hallyn

Quoted 8 lines · Oleg Nesterov

	static inline uid_t good_name(struct task_struct *from, struct task_struct *to)
	{
		// the caller does rcu_read_lock() if needed
		const struct cred *from_cred = __task_cred(from);
		return user_ns_map_uid(task_cred_xxx(to, user_ns),
					from_cred, from_cred->uid);
	}

Quoted 3 lines

That looks great, thanks. I couldn't think it up myself, but now that I see it in your email, I see this would be very valuable in helping make this code more readable :)

Damn ;) The problem is, "the caller does rcu_read_lock() if needed" can't shut up __rcu_dereference_check(). current_cred() passes c == true to do this.

This reminds me, __task_cred()->task_is_dead() should go away. Probably we can replace it with (__t == current). Otherwise send_signal() needs rcu_read_lock() to avoid the warning from lockdep, or the helper needs the unconditional rcu_read_lock().

From 8f92054e commit:

Unfortunately, we can't kill task_is_dead() right now, it has already found the users in drivers/staging/, and I bet the usage is wrong.

(add more cc's)

Quoted 3 lines · Oleg Nesterov

Unfortunately, we can't kill task_is_dead() right now, it has already found the users in drivers/staging/, and I bet the usage is wrong.

It is used by drivers/staging/usbip/

For what? The code:

	if (vdev->ud.tcp_rx && !task_is_dead(vdev->ud.tcp_rx))
		kthread_stop(vdev->ud.tcp_rx);

And how task_is_dead() can help? This helper is really "special", it shouldn't be used anyway. But why do we check ->exit_state? Without tasklist the check is racy anyway, the task can exit right after the check.

And. It is safe to use kthread_stop(t) even if t has already exited.

OK, this was added by 8547d4cc2b616e4f1dafebe2c673fc986422b506 "Staging: usbip: vhci-hcd: Do not kill already dead RX/TX kthread"

When unbinding a device on the host which was still attached on the client, I got a NULL pointer dereference on the client.

Where?

This turned out
to be due to kthread_stop() being called on an already dead kthread.

This should work.

I'm afraid this can only fix the symptom. Probably, the problem is that we do not have the reference and thus even task_is_dead(t) is not safe.

This kthread was created by kthread_run(). If it exits, nothing protects this task_struct.

In any case, please do not use ->exit_state. It should not be used outside of exit.c/etc paths, "exit_state != 0" means "exit_notify() was called".

Quoted 37 lines · Oleg Nesterov

(add more cc's)

Quoted 3 lines · Oleg Nesterov

Unfortunately, we can't kill task_is_dead() right now, it has already found the users in drivers/staging/, and I bet the usage is wrong.

It is used by drivers/staging/usbip/

For what? The code:

	if (vdev->ud.tcp_rx && !task_is_dead(vdev->ud.tcp_rx))
		kthread_stop(vdev->ud.tcp_rx);

And how task_is_dead() can help? This helper is really "special", it shouldn't be used anyway. But why do we check ->exit_state? Without tasklist the check is racy anyway, the task can exit right after the check.

And. It is safe to use kthread_stop(t) even if t has already exited.

OK, this was added by 8547d4cc2b616e4f1dafebe2c673fc986422b506 "Staging: usbip: vhci-hcd: Do not kill already dead RX/TX kthread"

When unbinding a device on the host which was still attached on the client, I got a NULL pointer dereference on the client.

Where?

This turned out
to be due to kthread_stop() being called on an already dead kthread.

This should work.

I'm afraid this can only fix the symptom. Probably, the problem is that we do not have the reference and thus even task_is_dead(t) is not safe.

Quoted 5 lines

This kthread was created by kthread_run(). If it exits, nothing protects this task_struct.

In any case, please do not use ->exit_state. It should not be used outside of exit.c/etc paths, "exit_state != 0" means "exit_notify() was called".

Patches to fix this up in this driver are always gladly appreciated :)

Quoted 45 lines · Greg KH

Quoted 42 lines · Oleg Nesterov

(add more cc's)

Quoted 3 lines · Oleg Nesterov

Unfortunately, we can't kill task_is_dead() right now, it has already found the users in drivers/staging/, and I bet the usage is wrong.

It is used by drivers/staging/usbip/

For what? The code:

	if (vdev->ud.tcp_rx && !task_is_dead(vdev->ud.tcp_rx))
		kthread_stop(vdev->ud.tcp_rx);

And how task_is_dead() can help? This helper is really "special", it shouldn't be used anyway. But why do we check ->exit_state? Without tasklist the check is racy anyway, the task can exit right after the check.

And. It is safe to use kthread_stop(t) even if t has already exited.

OK, this was added by 8547d4cc2b616e4f1dafebe2c673fc986422b506 "Staging: usbip: vhci-hcd: Do not kill already dead RX/TX kthread"

When unbinding a device on the host which was still attached on the client, I got a NULL pointer dereference on the client.

Where?

This turned out
to be due to kthread_stop() being called on an already dead kthread.

This should work.

I'm afraid this can only fix the symptom. Probably, the problem is that we do not have the reference and thus even task_is_dead(t) is not safe.

This kthread was created by kthread_run(). If it exits, nothing protects this task_struct.

In any case, please do not use ->exit_state. It should not be used outside of exit.c/etc paths, "exit_state != 0" means "exit_notify() was called".

Quoted 1 line

Patches to fix this up in this driver are always gladly appreciated :)

OK, since nobody cares, probably I should make the patch even if I don't understand this code at all and can't test the change.

But, Tobias, may be you can explain what this task_is_dead() check was supposed to do?

Quoted 6 lines · Oleg Nesterov

OK, since nobody cares, probably I should make the patch even if I don't understand this code at all and can't test the change.

But, Tobias, may be you can explain what this task_is_dead() check was supposed to do?

As mentioned in the commit message, this was needed for me to work around a NULL pointer dereference I got during unbinding

(I only
experienced this behaviour on the nios2 platform though, couldn't reproduce it on e.g. x86_64).

I wasn't really familiar with the codebase of usbip (and still am not) but came up with the fix by more or less blindly copying what the opposite side is checking for in stub_shutdown_connection(). This fixed the behaviour for me and seemed legitimate as it was done equally there.

Quoted 6 lines · Tobias Klauser

Quoted 4 lines · Oleg Nesterov

OK, since nobody cares, probably I should make the patch even if I don't understand this code at all and can't test the change.

Quoted 5 lines

Quoted 2 lines

But, Tobias, may be you can explain what this task_is_dead() check was supposed to do?

As mentioned in the commit message, this was needed for me to work around a NULL pointer dereference I got during unbinding

Where? OK, I guess you do not remember the trace ;)

Quoted 3 lines

(I only
experienced this behaviour on the nios2 platform though, couldn't reproduce it on e.g. x86_64).

OK,

Quoted 4 lines

I wasn't really familiar with the codebase of usbip (and still am not) but came up with the fix by more or less blindly copying what the opposite side is checking for in stub_shutdown_connection(). This fixed the behaviour for me and seemed legitimate as it was done equally there.

But this looks "obviously wrong", and afaics just hides the problem. Not to mention this check is racy, it is simply unsafe to dereference this task_struct if the kthread has already exited.

At first glance we need something like the patch below (and stub_dev.c needs the same fix). It assumes that:

- vhci_shutdown_connection() is the only caller of kthread_stop(), iow nobody else does kthread_stop(tcp_*x)

- we can't leak the task_struct, vhci_shutdown_connection() should be called in any case at some point.

I'll try to grep more, but perhaps you can ack my understanding?

Looks OK to me at a first glance. I'll try to dig up my test system where I was able to produce that NULL pointer dereference and test your patch there and will then let you know my results.

From 8bec01c69c2b523e93432d36f9e211b6ec4a6771 Mon Sep 17 00:00:00 2001

Quoted 4 lines · Tobias Klauser

Looks OK to me at a first glance. I'll try to dig up my test system where I was able to produce that NULL pointer dereference and test your patch there and will then let you know my results.

Oh, great. Please find the patch below.

Of course it wasn't tested at all ;)

Quoted 7 lines · Oleg Nesterov

Quoted 4 lines · Tobias Klauser

Looks OK to me at a first glance. I'll try to dig up my test system where I was able to produce that NULL pointer dereference and test your patch there and will then let you know my results.

Quoted 3 lines

Oh, great. Please find the patch below.

Of course it wasn't tested at all ;)

Cough... Sorry, but another ping ;)

Quoted 1 line

From 8f92054e commit:

Instead, add the following validation condition:

task->exit_state >= 0

to permit the access if the target task is dead and therefore unable to change its own credentials.

OK, but afaics currently this can only help wait_task_zombie() which calls __task_cred() without rcu lock.

Remove this validation and change wait_task_zombie() to use task_uid() instead. This means we do rcu_read_lock() only to shut up the lockdep, but we already do the same in, say, wait_task_stopped().

Unfortunately, we can't kill task_is_dead() right now, it has already found the users in drivers/staging/, and I bet the usage is wrong.

From a first quick scan...

Signed-off-by Oleg Nesterov <[email protected]>

---

 include/linux/cred.h |    3 +--
 kernel/exit.c        |    2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

include/linux/cred.h renamed from include/linux/cred.h~1_kill_task_is_dead +0 -0

 	})
 
 /**

kernel/exit.c renamed from kernel/exit.c~1_kill_task_is_dead +1 -1

@@ -1191,7 +1191,7 @@ static int wait_task_zombie(struct wait_
 	unsigned long state;
 	int retval, status, traced;
 	pid_t pid = task_pid_vnr(p);
-	uid_t uid = __task_cred(p)->uid;
+	uid_t uid = task_uid(p);
 	struct siginfo __user *infop;
 
 	if (!likely(wo->wo_flags & WEXITED))

include/linux/cred.h static inline void put_cred(const struct · line 287

Quoted 10 lines · Oleg Nesterov

include/linux/cred.h renamed from include/linux/cred.h~1_kill_task_is_dead +1 -2

@@ -284,8 +284,7 @@ static inline void put_cred(const struct
 #define __task_cred(task)						\
 	({								\
 		const struct task_struct *__t = (task);			\
-		rcu_dereference_check(__t->real_cred,			\
-				      task_is_dead(__t));		\
+		rcu_dereference_check(__t->real_cred, 0);		\

The "0" above will make lockdep-RCU complain unconditionally. My guess is that you want rcu_dereference_raw().

Quoted 15 lines · Paul E. McKenney

include/linux/cred.h static inline void put_cred(const struct · line 287

Quoted 10 lines · Oleg Nesterov

include/linux/cred.h renamed from include/linux/cred.h~1_kill_task_is_dead +1 -2

@@ -284,8 +284,7 @@ static inline void put_cred(const struct
 #define __task_cred(task)						\
 	({								\
 		const struct task_struct *__t = (task);			\
-		rcu_dereference_check(__t->real_cred,			\
-				      task_is_dead(__t));		\
+		rcu_dereference_check(__t->real_cred, 0);		\

The "0" above will make lockdep-RCU complain unconditionally. My guess is that you want rcu_dereference_raw().

Hmm. I hope you are wrong this time ;)

rcu_dereference_check() checks rcu_read_lock_held(). IOW, with this change __task_cred() always requires rcu_read_lock(), and this is what the patch wants.

The next one adds " || (task == current" to the rcu_read_lock_held() check above.

From 8f92054e commit:

Instead, add the following validation condition:

task->exit_state >= 0

to permit the access if the target task is dead and therefore unable to change its own credentials.

OK, but afaics currently this can only help wait_task_zombie() which calls __task_cred() without rcu lock.

Remove this validation and change wait_task_zombie() to use task_uid() instead. This means we do rcu_read_lock() only to shut up the lockdep, but we already do the same in, say, wait_task_stopped().

Unfortunately, we can't kill task_is_dead() right now, it has already found the users in drivers/staging/, and I bet the usage is wrong.

Signed-off-by Oleg Nesterov <[email protected]>

Change __task_cred(task) to accept "task == current" without rcu_read_lock_held(). This is what current_cred() currently does, and with this change __task_cred() becomes more flexible/usable.

Signed-off-by Oleg Nesterov <[email protected]>

---

 include/linux/cred.h |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

include/linux/cred.h renamed from include/linux/cred.h~2_task_cred_ck_current +2 -3

@@ -268,8 +268,7 @@ static inline void put_cred(const struct
  * Access the subjective credentials of the current task.  RCU-safe,
  * since nobody else can modify it.
  */
-#define current_cred() \
-	rcu_dereference_protected(current->cred, 1)
+#define current_cred()	__task_cred(current)
 
 /**
  * __task_cred - Access a task's objective credentials

@@ -284,7 +283,7 @@ static inline void put_cred(const struct
 #define __task_cred(task)						\
 	({								\
 		const struct task_struct *__t = (task);			\
-		rcu_dereference_check(__t->real_cred, 0);		\
+		rcu_dereference_check(__t->real_cred, (__t == current));\
 	})
 
 /**

Quoted 4 lines · Oleg Nesterov

Change __task_cred(task) to accept "task == current" without rcu_read_lock_held(). This is what current_cred() currently does, and with this change __task_cred() becomes more flexible/usable.

Quoted 1 line

Signed-off-by Oleg Nesterov <[email protected]>

So to check whether I've got this straight, the original __task_cred() was allowing rcu read lock to not be held for a non-running task, but required rcu read lock if task was running? With these two patches, rcu read lock will not be needed if task == current?

If so, then that sounds good to me, and an unconditional rcu_read_lock() at wait_task_zombie() seems better than at send_signal()...

Change __task_cred(task) to accept "task == current" without rcu_read_lock_held(). This is what current_cred() currently does, and with this change __task_cred() becomes more flexible/usable.

Signed-off-by Oleg Nesterov <[email protected]>

Quoted 5 lines · Oleg Nesterov

Change __task_cred(task) to accept "task == current" without rcu_read_lock_held(). This is what current_cred() currently does, and with this change __task_cred() becomes more flexible/usable.

Signed-off-by Oleg Nesterov <[email protected]>

NAK!

If you compare carefully:

include/linux/cred.h static inline void put_cred(const struct · line 272

Quoted 1 line

include/linux/cred.h renamed from include/linux/cred.h~2_task_cred_ck_current +0 -1

-	rcu_dereference_protected(current->cred, 1)

and:

include/linux/cred.h static inline void put_cred(const struct · line 287

Quoted 1 line

include/linux/cred.h renamed from include/linux/cred.h~2_task_cred_ck_current +0 -1

-		rcu_dereference_check(__t->real_cred, 0);		\

you'll notice they aren't quite the same in one very fundamental way.

Quoted 7 lines

include/linux/cred.h static inline void put_cred(const struct · line 272

Quoted 1 line

include/linux/cred.h renamed from include/linux/cred.h~2_task_cred_ck_current +0 -1

-	rcu_dereference_protected(current->cred, 1)

and:

include/linux/cred.h static inline void put_cred(const struct · line 287

Quoted 1 line

include/linux/cred.h renamed from include/linux/cred.h~2_task_cred_ck_current +0 -1

-		rcu_dereference_check(__t->real_cred, 0);		\

you'll notice they aren't quite the same in one very fundamental way.

Do you mean that this patch adds the unnecessary ACCESS_ONCE + smp_read_barrier_depends() to current_cred() or I missed something else?

Quoted 8 lines · Oleg Nesterov

Quoted 7 lines

include/linux/cred.h static inline void put_cred(const struct · line 272

Quoted 1 line

include/linux/cred.h renamed from include/linux/cred.h~2_task_cred_ck_current +0 -1

-	rcu_dereference_protected(current->cred, 1)

and:

include/linux/cred.h static inline void put_cred(const struct · line 287

Quoted 1 line

include/linux/cred.h renamed from include/linux/cred.h~2_task_cred_ck_current +0 -1

-		rcu_dereference_check(__t->real_cred, 0);		\

you'll notice they aren't quite the same in one very fundamental way.

Quoted 3 lines

Do you mean that this patch adds the unnecessary ACCESS_ONCE + smp_read_barrier_depends() to current_cred() or I missed something else?

Something else. The current_cred() uses ->cred:

Quoted 9 lines · Serge E. Hallyn

__send_signal: convert the uid being sent in SI_USER to the target task's user namespace.

do_notify_parent and do_notify_parent_cldstop: map task's uid to parent's
user namespace

ptrace_signal: map parent's uid into current's user namespace before including in signal to current.

And all of them follow the same pattern,

kernel/signal.c static int __send_signal(int sig, struct siginfo *info, struct task_struct *t, · line 1078

Quoted 7 lines

kernel/signal.c +2 -1

@@ -1073,7 +1074,8 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 			q->info.si_code = SI_USER;
 			q->info.si_pid = task_tgid_nr_ns(current,
 							task_active_pid_ns(t));
-			q->info.si_uid = current_uid();
+			q->info.si_uid = user_ns_map_uid(task_cred_xxx(t, user_ns),
+					current_cred(), current_uid());

Up to you, but may be we can add a helper? Something like

	static inline uid_t good_name(struct task_struct *from, struct task_struct *to)
	{
		// the caller does rcu_read_lock() if needed
		const struct cred *from_cred = __task_cred(from);
		return user_ns_map_uid(task_cred_xxx(to, user_ns),
					from_cred, from_cred->uid);
	}

As for __send_signal() in particular, I guess we could do

		q->info.si_uid = from_ancestor_ns ? 0 : current_uid();

Quoted 4 lines · Oleg Nesterov

As for __send_signal() in particular, I guess we could do

		q->info.si_uid = from_ancestor_ns ? 0 : current_uid();

But wait - can't __send_signal end up being triggered through fcntl F_SETSIG+F_SETOWN to another but none-ancestor namespace?

do actually need to call the full user_ns_map_uid() there. However, the uid sent in ptrace_signal() can (if i'm thinking right) be set using

	info->si_uid = (current_user_ns() == parent_user_ns()) ?
			0 : current_uid();

Not much of a win performance-wise, but perhaps better by making it clear that those are the only possibilities (and overflowuid is not possible).

Quoted 5 lines · Serge Hallyn

Quoted 3 lines · Oleg Nesterov

As for __send_signal() in particular, I guess we could do

Quoted 4 lines

Quoted 1 line

		q->info.si_uid = from_ancestor_ns ? 0 : current_uid();

But wait - can't __send_signal end up being triggered through fcntl F_SETSIG+F_SETOWN to another but none-ancestor namespace?

I didn't really check, but afaics send_sigio_to_task/etc never use SEND_SIG_NOINFO. And they shouldn't, there is no the sending process.

Quoted 4 lines

do actually need to call the full user_ns_map_uid() there. However, the uid sent in ptrace_signal() can (if i'm thinking right) be set using

Quoted 6 lines

	info->si_uid = (current_user_ns() == parent_user_ns()) ?
			0 : current_uid();

Not much of a win performance-wise, but perhaps better by making it clear that those are the only possibilities (and overflowuid is not possible).

I think you are right right, except s/==/!=/.

This is like send_signal()
which can only send the signal down or to the same namespace.

I guess ptrace_signal() can even do

	info->si_pid = task_pid_vnr(current->parent);

	if (info->si_pid)	// same namespace
		info->si_uid = current_uid();
	else			// can't see the tracer, we are from the sub-namespace
		info->si_uid = 0;

Whatever you like more.

And just in case let me repeat, I won't argue if you use user_ns_map_uid/helper for consistency. Without CONFIG_USER_NS it is even better performance-wise (even if we add a helper I guess).

Quoted 3 lines · Oleg Nesterov

I didn't really check, but afaics send_sigio_to_task/etc never use SEND_SIG_NOINFO. And they shouldn't, there is no the sending process.

(I'm sure you're right -

Quoted 9 lines

I guess ptrace_signal() can even do

	info->si_pid = task_pid_vnr(current->parent);

	if (info->si_pid)	// same namespace
		info->si_uid = current_uid();
	else			// can't see the tracer, we are from the sub-namespace
		info->si_uid = 0;

Quoted 1 line

Whatever you like more.

Except you don't have to split pid and user namespaces at the same time.

__send_signal: convert the uid being sent in SI_USER to the target task's user namespace.

do_notify_parent and do_notify_parent_cldstop: map task's uid to parent's
user namespace

ptrace_signal: map parent's uid into current's user namespace before including in signal to current.

Quoted 4 lines · Serge E. Hallyn

__send_signal: convert the uid being sent in SI_USER to the target task's user namespace.

Quoted 5 lines

do_notify_parent and do_notify_parent_cldstop: map task's uid to parent's
user namespace

ptrace_signal: map parent's uid into current's user namespace before including in signal to current.

Btw, what about the other users of ->si_uid? Say, kill() or tkill().

Looks like, we need a lot of complications to make this correct...

As for send_signal(), may be we can simply do

	--- x/kernel/signal.c
	+++ x/kernel/signal.c
	@@ -1084,10 +1084,13 @@ static int __send_signal(int sig, struct
				break;
			default:
				copy_siginfo(&q->info, info);
	-			if (from_ancestor_ns)
	-				q->info.si_pid = 0;
				break;
			}
	+
	+		if (unlikely(from_ancestor_ns)) {
	+				q->info.si_pid = 0;
	+				q->info.si_uid = 0;
	+		}
		} else if (!is_si_special(info)) {
			if (sig >= SIGRTMIN && info->si_code != SI_USER) {
				/*

?

Yes, this "breaks" sys_rt_sigqueueinfo() evem more if it is used to send the signal to the sub-namespace.

Quoted 8 lines · Oleg Nesterov

Quoted 7 lines · Serge E. Hallyn

__send_signal: convert the uid being sent in SI_USER to the target task's user namespace.

do_notify_parent and do_notify_parent_cldstop: map task's uid to parent's
user namespace

Quoted 4 lines

Quoted 2 lines

ptrace_signal: map parent's uid into current's user namespace before including in signal to current.

Btw, what about the other users of ->si_uid? Say, kill() or tkill().

Well, they end up calling __send_signal().

But you sound more alarmed than that.

Quoted 13 lines · Serge E. Hallyn

Quoted 11 lines · Oleg Nesterov

Quoted 9 lines · Serge E. Hallyn

__send_signal: convert the uid being sent in SI_USER to the target task's user namespace.

do_notify_parent and do_notify_parent_cldstop: map task's uid to parent's
user namespace

ptrace_signal: map parent's uid into current's user namespace before including in signal to current.

Quoted 3 lines

Quoted 1 line

Btw, what about the other users of ->si_uid? Say, kill() or tkill().

Well, they end up calling __send_signal().

Sure. But your patch changes only the __send_signal(SEND_SIG_NOINFO). Note that SEND_SIG_NOINFO is rarely used, and only by kernel.

Quoted 15 lines · Serge E. Hallyn

Quoted 12 lines · Oleg Nesterov

Quoted 9 lines · Serge E. Hallyn

__send_signal: convert the uid being sent in SI_USER to the target task's user namespace.

do_notify_parent and do_notify_parent_cldstop: map task's uid to parent's
user namespace

ptrace_signal: map parent's uid into current's user namespace before including in signal to current.

Btw, what about the other users of ->si_uid? Say, kill() or tkill().

Quoted 1 line

Well, they end up calling __send_signal().

Sure. But your patch changes only the __send_signal(SEND_SIG_NOINFO). Note that SEND_SIG_NOINFO is rarely used, and only by kernel.

Quoted 1 line

But you sound more alarmed than that.

I am ;)

It looks like I can fix all the
cases

at send_signal()

by checking whether si_fromuser(info)

Quoted 3 lines · Serge E. Hallyn

It looks like I can fix all the
cases

except ptrace_signal(). Although we can simply ignore this case, imho.

Quoted 1 line

at send_signal()

Yes, I was thinking about this too but didn't have the time to check if this can really work.

Quoted 1 line

by checking whether si_fromuser(info)

I am not sure... sys_rt_queueinfo() is nasty. Plus we have to handle the "fromkernel" case too. May be we can ignore this too.

Quoted 6 lines · Oleg Nesterov

Quoted 3 lines · Serge E. Hallyn

It looks like I can fix all the
cases

except ptrace_signal(). Although we can simply ignore this case, imho.

ptrace_signal() calls send_signal() though.

Quoted 4 lines

Quoted 1 line

by checking whether si_fromuser(info)

I am not sure... sys_rt_queueinfo() is nasty. Plus we have to handle the "fromkernel" case too. May be we can ignore this too.

sys_rt_tgsigqueueinfo() still seems to go through send_signal().

Quoted 7 lines · Serge E. Hallyn

Quoted 5 lines · Oleg Nesterov

Quoted 3 lines · Serge E. Hallyn

It looks like I can fix all the
cases

Quoted 3 lines

Quoted 1 line

except ptrace_signal(). Although we can simply ignore this case, imho.

ptrace_signal() calls send_signal() though.

Confused... I meant the "if (signr != info->si_signo)" case. This is simple, and I only meant that this case is not that important.

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

by checking whether si_fromuser(info)

I am not sure... sys_rt_queueinfo() is nasty. Plus we have to handle the "fromkernel" case too. May be we can ignore this too.

sys_rt_tgsigqueueinfo() still seems to go through send_signal().

Yes. But how can you fix si_uid? We do not even know if it exists. Please look at siginfo/_uid, there is a union. We can't know what the caller of sys_rt_sigqueueinfo() puts in this location.

Quoted 10 lines · Oleg Nesterov

Quoted 9 lines · Serge E. Hallyn

Quoted 6 lines · Oleg Nesterov

Quoted 3 lines · Serge E. Hallyn

It looks like I can fix all the
cases

except ptrace_signal(). Although we can simply ignore this case, imho.

Quoted 4 lines

Quoted 1 line

ptrace_signal() calls send_signal() though.

Confused... I meant the "if (signr != info->si_signo)" case. This is simple, and I only meant that this case is not that important.

Yes, that's the case I was talking about. That then proceeds through send_signal().

The whole new patch (so far only compile-tested) is below.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

by checking whether si_fromuser(info)

I am not sure... sys_rt_queueinfo() is nasty. Plus we have to handle the "fromkernel" case too. May be we can ignore this too.

Quoted 5 lines

Quoted 1 line

sys_rt_tgsigqueueinfo() still seems to go through send_signal().

Yes. But how can you fix si_uid? We do not even know if it exists. Please look at siginfo/_uid, there is a union. We can't know what the caller of sys_rt_sigqueueinfo() puts in this location.

But it's a union alongside the pid.

Quoted 14 lines · Serge E. Hallyn

Quoted 12 lines · Oleg Nesterov

Quoted 10 lines · Serge E. Hallyn

Quoted 6 lines · Oleg Nesterov

Quoted 3 lines · Serge E. Hallyn

It looks like I can fix all the
cases

except ptrace_signal(). Although we can simply ignore this case, imho.

ptrace_signal() calls send_signal() though.

Quoted 5 lines

Quoted 2 lines

Confused... I meant the "if (signr != info->si_signo)" case. This is simple, and I only meant that this case is not that important.

Yes, that's the case I was talking about. That then proceeds through send_signal().

It doesn't?

I am even more confuused. Anyway, your patch adds map_cred_ns() into ptrace_signal().

Quoted 1 line

The whole new patch (so far only compile-tested) is below.

Perhaps I missed something, but it looks overcomplicated.

I was thinking
about the (uncompiled/untested) simple patch below (it ignores ptrace_signal for clarity).

And note that this way we do not need to modify do_notify_parent*() or ipc/mqueue.c:__do_notify() (your patch doesn't cover the latter). Unless I missed something of course.

And we do not need to handle the SEND_SIG_NOINFO case separately.

However, we still have the problems with sigqueueinfo,

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

by checking whether si_fromuser(info)

I am not sure... sys_rt_queueinfo() is nasty. Plus we have to handle the "fromkernel" case too. May be we can ignore this too.

sys_rt_tgsigqueueinfo() still seems to go through send_signal().

Yes. But how can you fix si_uid? We do not even know if it exists. Please look at siginfo/_uid, there is a union. We can't know what the caller of sys_rt_sigqueueinfo() puts in this location.

Quoted 1 line

But it's a union alongside the pid.

Again, I do not understand... Yes, we have the same problem with

	if (from_ancestor_ns)
		q->info.si_pid = 0;

This was discussed, we do not know what we can do.

My point was, this
change is not sigqueueinfo-friendly too.

Oleg.

kernel/signal.c +21 -0

@@ -1019,6 +1019,27 @@ static inline int legacy_queue(struct si
 	return (sig < SIGRTMIN) && sigismember(&signals->signal, sig);
 }
 
+static inline fixup_uid(struct siginfo *info, struct task_struct *t)
+{
+#ifdef CONFIG_USER_NS
+	if (current_user_ns() == task_cred_xxx(t, user_ns)))
+#endif
+		return;
+
+	if (SI_FROMKERNEL(info))
+		switch (info->si_code & __SI_MASK) {
+			default:
+				return;
+
+			case __SI_CHLD:
+			case __SI_MESGQ:
+				break;
+		}
+
+	info->si_uid = user_ns_map_uid(task_cred_xxx(t, user_ns),
+					current_cred(), info->si_uid);
+}
+
 static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 			int group, int from_ancestor_ns)
 {

Quoted 6 lines · Oleg Nesterov

Quoted 3 lines · Serge E. Hallyn

Yes, that's the case I was talking about. That then proceeds through send_signal().

It doesn't?

No, I was saying it *does*.

Quoted 30 lines

kernel/signal.c +23 -0

+static inline fixup_uid(struct siginfo *info, struct task_struct *t)
+{
+#ifdef CONFIG_USER_NS
+	if (current_user_ns() == task_cred_xxx(t, user_ns)))
+#endif
+		return;
+
+	if (SI_FROMKERNEL(info))
+		switch (info->si_code & __SI_MASK) {
+			default:
+				return;
+
+			case __SI_CHLD:
+			case __SI_MESGQ:
+				break;
+		}
+
+	info->si_uid = user_ns_map_uid(task_cred_xxx(t, user_ns),
+					current_cred(), info->si_uid);
+}
+
 static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 			int group, int from_ancestor_ns)
 {

@@ -1088,6 +1109,9 @@ static int __send_signal(int sig, struct
 				q->info.si_pid = 0;
 			break;
 		}
+
+		fixup_uid(info, t);

kernel/signal.c static int __send_signal(int sig, struct · line 1117

Quoted 4 lines

kernel/signal.c +1 -0

+
 	} else if (!is_si_special(info)) {
 		if (sig >= SIGRTMIN && info->si_code != SI_USER) {
 			/*

It certainly is much simpler. I'll take some time to walk through all of send_signal again and make sure I understand what it does in all the cases.

Quoted 10 lines · Serge Hallyn

Quoted 6 lines · Oleg Nesterov

Quoted 3 lines · Serge E. Hallyn

Yes, that's the case I was talking about. That then proceeds through send_signal().

It doesn't?

No, I was saying it *does*.

But it doesn't ;)

Serge, there is some misunderstanding. And I do not know who is confused, me or your.

ptrace_signal() simply fills *info with some "random" data before processing the signal. It doesn't pass this info to send_signal().

If the debuggure wants something meaningfull in *info, it should fill it itself via PTRACE_SETSIGINFO. This handles the case when the debugger doesn't really care and only changes signr.

Quoted 30 lines

Quoted 30 lines

kernel/signal.c +23 -0

+static inline fixup_uid(struct siginfo *info, struct task_struct *t)
+{
+#ifdef CONFIG_USER_NS
+	if (current_user_ns() == task_cred_xxx(t, user_ns)))
+#endif
+		return;
+
+	if (SI_FROMKERNEL(info))
+		switch (info->si_code & __SI_MASK) {
+			default:
+				return;
+
+			case __SI_CHLD:
+			case __SI_MESGQ:
+				break;
+		}
+
+	info->si_uid = user_ns_map_uid(task_cred_xxx(t, user_ns),
+					current_cred(), info->si_uid);
+}
+
 static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 			int group, int from_ancestor_ns)
 {

@@ -1088,6 +1109,9 @@ static int __send_signal(int sig, struct
 				q->info.si_pid = 0;
 			break;
 		}
+
+		fixup_uid(info, t);

Quoted 8 lines

kernel/signal.c static int __send_signal(int sig, struct · line 1117

Quoted 4 lines

kernel/signal.c +1 -0

+
 	} else if (!is_si_special(info)) {
 		if (sig >= SIGRTMIN && info->si_code != SI_USER) {
 			/*

It certainly is much simpler. I'll take some time to walk through all of send_signal again and make sure I understand what it does in all the cases.

Thanks.

As for ptrace_signal(), it can use the same helper too. Once again, debugger can use PTRACE_SETSIGINFO, if we really want to fixup si_uid we should do this even if signr == info->si_signo. OTOH, we do not know what debugger puts in this chunk of memory. This is like sigqueueinfo().

Or we can simply leave this code as is. I do not think this si_uid (in this case) is really important.

Quoted 3 lines · Oleg Nesterov

ptrace_signal() simply fills *info with some "random" data before processing the signal. It doesn't pass this info to send_signal().

Oh, well not always, but it does in the case where

	sigismember(&current->blocked, signr);

But I suppose that's not a common path :)

kernel/signal.c +3 -0

@@ -1019,6 +1019,27 @@ static inline int legacy_queue(struct si
 	return (sig < SIGRTMIN) && sigismember(&signals->signal, sig);
 }
 
+static inline fixup_uid(struct siginfo *info, struct task_struct *t)
+{
+#ifdef CONFIG_USER_NS

kernel/signal.c +11 -0

+			case __SI_MESGQ:
+				break;
+		}
+
+	info->si_uid = user_ns_map_uid(task_cred_xxx(t, user_ns),
+					current_cred(), info->si_uid);
+}
+
 static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 			int group, int from_ancestor_ns)
 {

@@ -1088,6 +1109,9 @@ static int __send_signal(int sig, struct
 				q->info.si_pid = 0;
 			break;
 		}
+
+		fixup_uid(info, t);
+
 	} else if (!is_si_special(info)) {
 		if (sig >= SIGRTMIN && info->si_code != SI_USER) {
 			/*

Quoted 6 lines · Oleg Nesterov

kernel/signal.c +0 -0

@@ -1019,6 +1019,27 @@ static inline int legacy_queue(struct si
 	return (sig < SIGRTMIN) && sigismember(&signals->signal, sig);
 }
 

kernel/signal.c static inline int legacy_queue(struct si · line 1034

Quoted 13 lines

kernel/signal.c +13 -0

+static inline fixup_uid(struct siginfo *info, struct task_struct *t)
+{
+#ifdef CONFIG_USER_NS
+	if (current_user_ns() == task_cred_xxx(t, user_ns)))
+#endif
+		return;
+
+	if (SI_FROMKERNEL(info))
+		switch (info->si_code & __SI_MASK) {
+			default:
+				return;
+
+			case __SI_CHLD:

If I'm reading this right, this will catch do_notify_parent, which is sending signals not from current.

this. For pids that case is being ignored by not using the __SI_MASK, so CLD_EXITED etc will cause from_ancestor_ns to be false. Could we do that here too, and then translate the pids in do_notify_parent?

Quoted 22 lines

kernel/signal.c +11 -0

+			case __SI_MESGQ:
+				break;
+		}
+
+	info->si_uid = user_ns_map_uid(task_cred_xxx(t, user_ns),
+					current_cred(), info->si_uid);
+}
+
 static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 			int group, int from_ancestor_ns)
 {

@@ -1088,6 +1109,9 @@ static int __send_signal(int sig, struct
 				q->info.si_pid = 0;
 			break;
 		}
+
+		fixup_uid(info, t);
+
 	} else if (!is_si_special(info)) {
 		if (sig >= SIGRTMIN && info->si_code != SI_USER) {
 			/*

Sorry, I missed this email...

Quoted 8 lines · Serge E. Hallyn

Quoted 6 lines · Oleg Nesterov

kernel/signal.c +0 -0

@@ -1019,6 +1019,27 @@ static inline int legacy_queue(struct si
 	return (sig < SIGRTMIN) && sigismember(&signals->signal, sig);
 }
 

Quoted 16 lines

kernel/signal.c static inline int legacy_queue(struct si · line 1034

Quoted 13 lines

kernel/signal.c +13 -0

+static inline fixup_uid(struct siginfo *info, struct task_struct *t)
+{
+#ifdef CONFIG_USER_NS
+	if (current_user_ns() == task_cred_xxx(t, user_ns)))
+#endif
+		return;
+
+	if (SI_FROMKERNEL(info))
+		switch (info->si_code & __SI_MASK) {
+			default:
+				return;
+
+			case __SI_CHLD:

If I'm reading this right, this will catch do_notify_parent, which is sending signals not from current.

Argh, indeed. So we need to modify the callers.

OTOH, this is only possible if the debugger notifies tracee->real_parent, or we reparent to /sbin/init.

Anyways, yes, I forgot about this. Thanks.

Oleg.

Quoted 12 lines · Serge E. Hallyn

Quoted 10 lines · Oleg Nesterov

Quoted 9 lines · Serge E. Hallyn

Quoted 6 lines · Oleg Nesterov

Quoted 3 lines · Serge E. Hallyn

It looks like I can fix all the
cases

except ptrace_signal(). Although we can simply ignore this case, imho.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

ptrace_signal() calls send_signal() though.

Confused... I meant the "if (signr != info->si_signo)" case. This is simple, and I only meant that this case is not that important.

Yes, that's the case I was talking about. That then proceeds through send_signal().

It doesn't? I am even more confuused. Anyway, your patch adds map_cred_ns() into ptrace_signal().

Quoted 1 line

The whole new patch (so far only compile-tested) is below.

Perhaps I missed something, but it looks overcomplicated. I was thinking about the (uncompiled/untested) simple patch below (it ignores ptrace_signal for clarity).

And note that this way we do not need to modify do_notify_parent*() or ipc/mqueue.c:__do_notify() (your patch doesn't cover the latter). Unless I missed something of course.

And we do not need to handle the SEND_SIG_NOINFO case separately.

However, we still have the problems with sigqueueinfo,

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

by checking whether si_fromuser(info)

I am not sure... sys_rt_queueinfo() is nasty. Plus we have to handle the "fromkernel" case too. May be we can ignore this too.

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

sys_rt_tgsigqueueinfo() still seems to go through send_signal().

Yes. But how can you fix si_uid? We do not even know if it exists. Please look at siginfo/_uid, there is a union. We can't know what the caller of sys_rt_sigqueueinfo() puts in this location.

But it's a union alongside the pid.

Again, I do not understand... Yes, we have the same problem with

	if (from_ancestor_ns)
		q->info.si_pid = 0;

This was discussed, we do not know what we can do. My point was, this change is not sigqueueinfo-friendly too.

Oleg.

kernel/signal.c +3 -0

@@ -1019,6 +1019,27 @@ static inline int legacy_queue(struct si
 	return (sig < SIGRTMIN) && sigismember(&signals->signal, sig);
 }
 
+static inline fixup_uid(struct siginfo *info, struct task_struct *t)
+{
+#ifdef CONFIG_USER_NS

kernel/signal.c +7 -0

+				break;
+		}
+
+	info->si_uid = user_ns_map_uid(task_cred_xxx(t, user_ns),
+					current_cred(), info->si_uid);
+}
+
 static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 			int group, int from_ancestor_ns)
 {

Quoted 4 lines · Oleg Nesterov

kernel/signal.c +3 -0

+static inline fixup_uid(struct siginfo *info, struct task_struct *t)
+{
+#ifdef CONFIG_USER_NS

kernel/signal.c static inline int legacy_queue(struct si · line 1034

Quoted 10 lines

kernel/signal.c +10 -0

+	if (current_user_ns() == task_cred_xxx(t, user_ns)))
+#endif
+		return;
+
+	if (SI_FROMKERNEL(info))
+		switch (info->si_code & __SI_MASK) {
+			default:
+				return;
+
+			case __SI_CHLD:

After looking a bit more, I really think the __SI_CHLD case just needs to always be converted at the callers (i.e. do_notify_parent).

kernel/signal.c static inline int legacy_queue(struct si · line 1035

Quoted 1 line

kernel/signal.c +1 -0

+			case __SI_MESGQ:

This can be done like this, but if this is going to be the only case of a SI_FROMKERNEL not being converted at the caller, and there is aiui only one caller (__do_notivy in ipc/mmqueue.c), it seems better to just have fixup_uid() always return for SI_FROMKERNEL(info).