Re: [linux-usb-devel] Re: [Security] [vendor-sec] [BUG/PATCH/RFC] Oops while completing async USB via usbdevio

[Harald Welte <laforge@gnumonks.org>]

On Fri, Sep 30, 2005 at 12:27:04PM -0700, Linus Torvalds wrote:
> 
> 
> On Fri, 30 Sep 2005, Chris Wright wrote:
> > 
> > Sorry, I missed the thread up to this, but this looks fundamentally
> > broken.  The kill_proc_info_as_uid() idea is not sufficient because more
> > than uid/euid are needed for permission check.  There's capabilities and
> > security labels.
> 
> Not for this particular USB use, there isn't. Since you can only send a 
> signal to yourself anyway, the uid/euid check is just testing that you're 
> still who you were.

please find the patch below.  It compiles, but I didn't yet have the
time to verify it makes the bug disappear and the async urb delivery is
still working.

I'll try to report whether it's working tomorrow.

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -30,6 +30,8 @@
  *  Revision history
  *    22.12.1999   0.1   Initial release (split from proc_usb.c)
  *    04.01.2000   0.2   Turned into its own filesystem
+ *    30.09.2005   0.3   Fix user-triggerable oops in async URB delivery
+ *    			 (CAN-2005-3055)
  */
 
 /*****************************************************************************/
@@ -58,7 +60,8 @@ static struct class *usb_device_class;
 struct async {
 	struct list_head asynclist;
 	struct dev_state *ps;
-	struct task_struct *task;
+	pid_t pid;
+	pid_t uid, euid;
 	unsigned int signr;
 	unsigned int ifnum;
 	void __user *userbuffer;
@@ -290,7 +293,8 @@ static void async_completed(struct urb *
 		sinfo.si_errno = as->urb->status;
 		sinfo.si_code = SI_ASYNCIO;
 		sinfo.si_addr = as->userurb;
-		send_sig_info(as->signr, &sinfo, as->task);
+		kill_proc_info_as_uid(as->signr, &sinfo, as->pid, as->uid, 
+				      as->euid);
 	}
         wake_up(&ps->wait);
 }
@@ -988,7 +992,9 @@ static int proc_do_submiturb(struct dev_
 		as->userbuffer = NULL;
 	as->signr = uurb->signr;
 	as->ifnum = ifnum;
-	as->task = current;
+	as->pid = current->pid;
+	as->uid = current->uid;
+	as->euid = current->euid;
 	if (!(uurb->endpoint & USB_DIR_IN)) {
 		if (copy_from_user(as->urb->transfer_buffer, uurb->buffer, as->urb->transfer_buffer_length)) {
 			free_async(as);
diff --git a/include/linux/sched.h b/include/linux/sched.h
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1018,6 +1018,7 @@ extern int force_sig_info(int, struct si
 extern int __kill_pg_info(int sig, struct siginfo *info, pid_t pgrp);
 extern int kill_pg_info(int, struct siginfo *, pid_t);
 extern int kill_proc_info(int, struct siginfo *, pid_t);
+extern int kill_proc_info_as_uid(int, struct siginfo *, pid_t, pid_t, pid_t);
 extern void do_notify_parent(struct task_struct *, int);
 extern void force_sig(int, struct task_struct *);
 extern void force_sig_specific(int, struct task_struct *);
diff --git a/kernel/signal.c b/kernel/signal.c
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -655,7 +655,8 @@ static int rm_from_queue(unsigned long m
  * Bad permissions for sending the signal
  */
 static int check_kill_permission(int sig, struct siginfo *info,
-				 struct task_struct *t)
+				 struct task_struct *t,
+				 pid_t uid, pid_t euid)
 {
 	int error = -EINVAL;
 	if (!valid_signal(sig))
@@ -665,8 +666,8 @@ static int check_kill_permission(int sig
 			(unsigned long)info != 2 && SI_FROMUSER(info)))
 	    && ((sig != SIGCONT) ||
 		(current->signal->session != t->signal->session))
-	    && (current->euid ^ t->suid) && (current->euid ^ t->uid)
-	    && (current->uid ^ t->suid) && (current->uid ^ t->uid)
+	    && (euid ^ t->suid) && (euid ^ t->uid)
+	    && (uid ^ t->suid) && (uid ^ t->uid)
 	    && !capable(CAP_KILL))
 		return error;
 
@@ -1132,7 +1133,24 @@ int group_send_sig_info(int sig, struct 
 	unsigned long flags;
 	int ret;
 
-	ret = check_kill_permission(sig, info, p);
+	ret = check_kill_permission(sig, info, p, current->uid, current->euid);
+	if (!ret && sig && p->sighand) {
+		spin_lock_irqsave(&p->sighand->siglock, flags);
+		ret = __group_send_sig_info(sig, info, p);
+		spin_unlock_irqrestore(&p->sighand->siglock, flags);
+	}
+
+	return ret;
+}
+
+static int group_send_sig_info_as_uid(int sig, struct siginfo *info, 
+				      struct task_struct *p, 
+				      pid_t uid, pid_t euid)
+{
+	unsigned long flags;
+	int ret;
+
+	ret = check_kill_permission(sig, info, p, current->uid, current->euid);
 	if (!ret && sig && p->sighand) {
 		spin_lock_irqsave(&p->sighand->siglock, flags);
 		ret = __group_send_sig_info(sig, info, p);
@@ -1192,6 +1210,22 @@ kill_proc_info(int sig, struct siginfo *
 	return error;
 }
 
+/* like kill_proc_info(), but doesn't use uid/euid of "current" */
+int
+kill_proc_info_as_uid(int sig, struct siginfo *info, pid_t pid,
+		      pid_t uid, pid_t euid)
+{
+	int error;
+	struct task_struct *p;
+
+	read_lock(&tasklist_lock);
+	p = find_task_by_pid(pid);
+	error = -ESRCH;
+	if (p)
+		error = group_send_sig_info_as_uid(sig, info, p, uid, euid);
+	read_unlock(&tasklist_lock);
+	return error;
+}
 
 /*
  * kill_something_info() interprets pid in interesting ways just like kill(2).
@@ -2290,7 +2324,8 @@ asmlinkage long sys_tgkill(int tgid, int
 	p = find_task_by_pid(pid);
 	error = -ESRCH;
 	if (p && (p->tgid == tgid)) {
-		error = check_kill_permission(sig, &info, p);
+		error = check_kill_permission(sig, &info, p, current->uid,
+					      current->euid);
 		/*
 		 * The null signal is a permissions and process existence
 		 * probe.  No signal is actually delivered.
@@ -2330,7 +2365,8 @@ sys_tkill(int pid, int sig)
 	p = find_task_by_pid(pid);
 	error = -ESRCH;
 	if (p) {
-		error = check_kill_permission(sig, &info, p);
+		error = check_kill_permission(sig, &info, p, current->uid,
+					      current->euid);
 		/*
 		 * The null signal is a permissions and process existence
 		 * probe.  No signal is actually delivered.
-- 
- Harald Welte <laforge@gnumonks.org>          	        http://gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
                                                  (ETSI EN 300 175-7 Ch. A6)

Attachment: pgpbnbugEAbCJ.pgp
Description: PGP signature