Skip to contentSkip to conversation

setpgid: should not accept ptraced childs

PATCH In review Oleg Nesterov 2 participants 4 messages Dec 11, 2005 – Dec 12, 2005 1 message not in the archive Scheduler

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Oleg Nesterov

author
#1 lore raw

PATCH 1/2 setpgid: should not accept ptraced childs

sys_setpgid() allows to change ->pgrp of ptraced childs.

'man setpgid' does not tell anything about that, so I consider this behaviour is a bug.

Signed-off-by Oleg Nesterov <[email protected]>
kernel/sys.c renamed from kernel/sys.c~ +1 -1
@@ -1106,7 +1106,7 @@ asmlinkage long sys_setpgid(pid_t pid, p
 	if (!thread_group_leader(p))
 		goto out;
 
-	if (p->parent == current || p->real_parent == current) {
+	if (p->real_parent == current) {
 		err = -EPERM;
 		if (p->signal->session != current->signal->session)
 			goto out;
author
#2 lore raw

PATCH 2/2 setpgid: should work for sub-threads

In reply to a message not in the archive

setpgid(0, pgid) or setpgid(forked_child_pid, pgid) does not work unless the calling process is a thread_group_leader().

'man setpgid' does not tell anything about that, so I consider this behaviour is a bug.

Signed-off-by Oleg Nesterov <[email protected]>
kernel/sys.c renamed from kernel/sys.c~ +6 -5
@@ -1083,10 +1083,11 @@ asmlinkage long sys_times(struct tms __u
 asmlinkage long sys_setpgid(pid_t pid, pid_t pgid)
 {
 	struct task_struct *p;
+	struct task_struct *group_leader = current->group_leader;
 	int err = -EINVAL;
 
 	if (!pid)
-		pid = current->pid;
+		pid = group_leader->pid;
 	if (!pgid)
 		pgid = pid;
 	if (pgid < 0)

@@ -1106,16 +1107,16 @@ asmlinkage long sys_setpgid(pid_t pid, p
 	if (!thread_group_leader(p))
 		goto out;
 
-	if (p->real_parent == current) {
+	if (p->real_parent == group_leader) {
 		err = -EPERM;
-		if (p->signal->session != current->signal->session)
+		if (p->signal->session != group_leader->signal->session)
 			goto out;
 		err = -EACCES;
 		if (p->did_exec)
 			goto out;
 	} else {
 		err = -ESRCH;
-		if (p != current)
+		if (p != group_leader)
 			goto out;
 	}
 

@@ -1127,7 +1128,7 @@ asmlinkage long sys_setpgid(pid_t pid, p
 		struct task_struct *p;
 
 		do_each_task_pid(pgid, PIDTYPE_PGID, p) {
-			if (p->signal->session == current->signal->session)
+			if (p->signal->session == group_leader->signal->session)
 				goto ok_pgid;
 		} while_each_task_pid(pgid, PIDTYPE_PGID, p);
 		goto out;
#3 lore raw

Re: [PATCH ? 2/2] setpgid: should work for sub-threads

[PATCH 1/1] setsid: should work for sub-threads

setsid() does not work unless the calling process is a thread_group_leader().

'man setpgid' does not tell anything about that, so I consider this behaviour is a bug.

Signed-off-by Oren Laadan <[email protected]>

---

(see similar patch for setpgid by Oleg Nesterov). This one also required modifying kernel/exit.c:__set_special_pids() (and it's prototype in sched.h).

Please review...

include/linux/sched.h +1 -1
index b0ad6f3..9ff6e79 100644
@@ -1003,7 +1003,7 @@ extern struct   mm_struct init_mm;
 #define find_task_by_pid(nr)	find_task_by_pid_type(PIDTYPE_PID, nr)
 extern struct task_struct *find_task_by_pid_type(int type, int pid);
 extern void set_special_pids(pid_t session, pid_t pgrp);
-extern void __set_special_pids(pid_t session, pid_t pgrp);
+extern void __set_special_pids(struct task_struct *tsk, pid_t session, pid_t pgrp);
 
 /* per-UID process charging. */
 extern struct user_struct * alloc_uid(uid_t);
kernel/exit.c +11 -13
index ee51568..d47931f 100644
@@ -256,26 +256,24 @@ static inline void reparent_to_init(void
 	switch_uid(INIT_USER);
 }
 
-void __set_special_pids(pid_t session, pid_t pgrp)
+void __set_special_pids(struct task_struct *tsk, pid_t session, pid_t pgrp)
 {
-	struct task_struct *curr = current;
-
-	if (curr->signal->session != session) {
-		detach_pid(curr, PIDTYPE_SID);
-		curr->signal->session = session;
-		attach_pid(curr, PIDTYPE_SID, session);
-	}
-	if (process_group(curr) != pgrp) {
-		detach_pid(curr, PIDTYPE_PGID);
-		curr->signal->pgrp = pgrp;
-		attach_pid(curr, PIDTYPE_PGID, pgrp);
+	if (tsk->signal->session != session) {
+		detach_pid(tsk, PIDTYPE_SID);
+		tsk->signal->session = session;
+		attach_pid(tsk, PIDTYPE_SID, session);
+	}
+	if (process_group(tsk) != pgrp) {
+		detach_pid(tsk, PIDTYPE_PGID);
+		tsk->signal->pgrp = pgrp;
+		attach_pid(tsk, PIDTYPE_PGID, pgrp);
 	}
 }
 
 void set_special_pids(pid_t session, pid_t pgrp)
 {
 	write_lock_irq(&tasklist_lock);
-	__set_special_pids(session, pgrp);
+	__set_special_pids(current, session, pgrp);
 	write_unlock_irq(&tasklist_lock);
 }
kernel/sys.c +7 -9
index bce933e..c907f88 100644
@@ -1207,24 +1207,22 @@ asmlinkage long sys_getsid(pid_t pid)
 
 asmlinkage long sys_setsid(void)
 {
+	struct task_struct *leader = current->group_leader;
 	struct pid *pid;
 	int err = -EPERM;
 
-	if (!thread_group_leader(current))
-		return -EINVAL;
-
 	down(&tty_sem);
 	write_lock_irq(&tasklist_lock);
 
-	pid = find_pid(PIDTYPE_PGID, current->pid);
+	pid = find_pid(PIDTYPE_PGID, leader->pid);
 	if (pid)
 		goto out;
 
-	current->signal->leader = 1;
-	__set_special_pids(current->pid, current->pid);
-	current->signal->tty = NULL;
-	current->signal->tty_old_pgrp = 0;
-	err = process_group(current);
+	leader->signal->leader = 1;
+	__set_special_pids(leader, leader->pid, leader->pid);
+	leader->signal->tty = NULL;
+	leader->signal->tty_old_pgrp = 0;
+	err = process_group(leader);
 out:
 	write_unlock_irq(&tasklist_lock);
 	up(&tty_sem);
author
#4 lore raw

Re: [PATCH ? 2/2] setpgid: should work for sub-threads

kernel/exit.c static inline void reparent_to_init(void · line 261

Quoted 5 lines · Oren Laadan
kernel/exit.c +1 -2
-void __set_special_pids(pid_t session, pid_t pgrp)
+void __set_special_pids(struct task_struct *tsk, pid_t session, pid_t pgrp)
  {
-       struct task_struct *curr = current;

Do we need to add the new paramater? I think we can just do:

- struct task_struct *curr = current;
+ struct task_struct *curr = current->group_leader;

and that's all. This function is called from daemonize() also, but the kernel thread is a thread group leader always.

Oleg.