Hi!
I've been working on some userspace usb drivers in which I use the
asynchronous usb devio URB delivery to usrerspace.
There have always been some bug reports of kernel oopses while
terminating a program that uses the driver.
Now I found out a way to relatively easy trigger that oops from pcscd
(part of pcsc-lite) on any kernel, at least tested with 2.6.8 to 2.6.14-rc2.
Call Trace:
[<c0103dbf>] show_stack+0x7f/0xa0
[<c0103f60>] show_registers+0x160/0x1c0
[<c0104156>] die+0xf6/0x1a0
[<c0112cd6>] do_page_fault+0x356/0x68f
[<c01039bf>] error_code+0x4f/0x54
[<c0125fa9>] send_sig_info+0x39/0x80
[<c023cfd9>] async_completed+0xa9/0xb0
[<c0237e31>] usb_hcd_giveback_urb+0x31/0x80
[<f8a21e84>] finish_urb+0x74/0x100 [ohci_hcd]
[<f8a22f88>] finish_unlinks+0x2b8/0x2e0 [ohci_hcd]
[<f8a23d1d>] ohci_irq+0x17d/0x190 [ohci_hcd]
[<c0237eb8>] usb_hcd_irq+0x38/0x70
[<c0139ab3>] handle_IRQ_event+0x33/0x70
[<c0139bcd>] __do_IRQ+0xdd/0x150
[<c010551c>] do_IRQ+0x1c/0x30
[<c010388a>] common_interrupt+0x1a/0x20
So what do we learn from that? That usb_hcd_giveback_urb() is called
from in_interrupt() context and calls async_completed().
async_completed() calls send_sig_info(), which in turn does a
spin_lock(&tasklist_lock) to protect itself from task_struct->sighand
from going away. However, the call to
"spin_lock_irqsave(task_struct->sighand->siglock)" causes an oops,
because "sighand" has disappeared.
If a process issues an URB from userspace and (starts to) terminate
before the URB comes back, we run into the issue described above. This
is because the urb saves a pointer to "current" when it is posted to the
device, but there's no guarantee that this pointer is still valid
afterwards.
This basically means that any userspace process with permissions to
any arbitrary USB device can Ooops any kernel.(!)
In fact, there are two separate issues:
1) the pointer to "current" can become invalid, since the task could be
completely gone when the URB completion comes back from the device.
This can be fixed by get_task_struct() / put_task_struct().
2) Even if the saved task pointer is still pointing to a valid task_struct,
task_struct->sighand could have gone meanwhile. Therefore, the USB
async URB completion handler needs to reliably check whether
task_struct->sighand is still valid or not. In order to prevent a
race with __exit_sighand(), it needs to grab a
read_lock(&tasklist_lock). This strategy seems to work, since the
send_sig_info() code uses the same protection.
However, we now need to export a __send_sig_info() function, one that
expects to be called with read_lock(&tasklist_lock) already held by
the caller. It's ugly, but I doubt there is a less invasive
solution.
Expected FAQ's:
a) Q: But grabbing references to "current" and delivering URB completion
via signals is totally invalid and broken in may ways !
A: Yes, but dozens of userspace apps/drivers rely on this broken API.
As long as there's no new, sane, usbdevio2, we cannot just rip it
out without a replacement.
b) Q: Why can't wa use the normal SIGIO mechanism?
A: Because the usbdevio API needs to pass a __user pointer to the URB
along the delivered signal.
c) Q: If this is a local DoS, why did you post it publicly?
A: Because I've already informed linux-usb-devel in May.
I suggest this (or any other) fix to be applied to both 2.6.14 final and
the stable series. I didn't yet investigate 2.4.x, but I think it is
likely to have the same problem.
Cheers,
--
- Harald Welte <[email protected]> http://gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
First off, thanks for providing a patch for this problem, it is real,
and has been known for a while (thanks to your debugging :)
On Sun, Sep 25, 2005 at 05:13:30PM +0200, Harald Welte wrote:
>
> I suggest this (or any other) fix to be applied to both 2.6.14 final and
> the stable series. I didn't yet investigate 2.4.x, but I think it is
> likely to have the same problem.
I agree, but I think we need an EXPORT_SYMBOL_GPL() for your newly
exported symbol, otherwise the kernel will not build if you have USB
built as a module.
thanks,
greg k-h
On Tue, Sep 27, 2005 at 01:04:15AM -0700, Greg KH wrote:
> First off, thanks for providing a patch for this problem, it is real,
> and has been known for a while (thanks to your debugging :)
>
> On Sun, Sep 25, 2005 at 05:13:30PM +0200, Harald Welte wrote:
> >
> > I suggest this (or any other) fix to be applied to both 2.6.14 final and
> > the stable series. I didn't yet investigate 2.4.x, but I think it is
> > likely to have the same problem.
>
> I agree, but I think we need an EXPORT_SYMBOL_GPL() for your newly
> exported symbol, otherwise the kernel will not build if you have USB
> built as a module.
Hm, it's even messier. With your patch, we get:
*** Warning: "__send_sig_info" [drivers/usb/core/usbcore.ko] undefined!
*** Warning: "__put_task_struct" [drivers/usb/core/usbcore.ko] undefined!
when the USB core is a module.
I can't pass judgement if we want to export both of these functions to
modules... Anyone else know?
thanks,
greg k-h
On Tue, Sep 27, 2005 at 01:04:15AM -0700, Greg KH wrote:
> First off, thanks for providing a patch for this problem, it is real,
> and has been known for a while (thanks to your debugging :)
>
> On Sun, Sep 25, 2005 at 05:13:30PM +0200, Harald Welte wrote:
> >
> > I suggest this (or any other) fix to be applied to both 2.6.14 final and
> > the stable series. I didn't yet investigate 2.4.x, but I think it is
> > likely to have the same problem.
>
> I agree, but I think we need an EXPORT_SYMBOL_GPL() for your newly
> exported symbol, otherwise the kernel will not build if you have USB
> built as a module.
Where's the actual patch? And no, we certainly shouldn't export anything
to put a task_struct.
On Tue, Sep 27, 2005 at 01:48:46PM +0100, Christoph Hellwig wrote:
> On Tue, Sep 27, 2005 at 01:04:15AM -0700, Greg KH wrote:
> > First off, thanks for providing a patch for this problem, it is real,
> > and has been known for a while (thanks to your debugging :)
> >
> > On Sun, Sep 25, 2005 at 05:13:30PM +0200, Harald Welte wrote:
> > >
> > > I suggest this (or any other) fix to be applied to both 2.6.14 final and
> > > the stable series. I didn't yet investigate 2.4.x, but I think it is
> > > likely to have the same problem.
> >
> > I agree, but I think we need an EXPORT_SYMBOL_GPL() for your newly
> > exported symbol, otherwise the kernel will not build if you have USB
> > built as a module.
>
> Where's the actual patch? And no, we certainly shouldn't export anything
> to put a task_struct.
Earlier in this thread, on these mailing lists.
I've included it below too.
thanks,
greg k-h
From: Harald Welte <[email protected]>
Subject: USB: fix oops while completing async USB via usbdevio
If a process issues an URB from userspace and (starts to) terminate
before the URB comes back, we run into the issue described above. This
is because the urb saves a pointer to "current" when it is posted to the
device, but there's no guarantee that this pointer is still valid
afterwards.
This basically means that any userspace process with permissions to
any arbitrary USB device can Ooops any kernel.(!)
In fact, there are two separate issues:
1) the pointer to "current" can become invalid, since the task could be
completely gone when the URB completion comes back from the device.
This can be fixed by get_task_struct() / put_task_struct().
2) Even if the saved task pointer is still pointing to a valid task_struct,
task_struct->sighand could have gone meanwhile. Therefore, the USB
async URB completion handler needs to reliably check whether
task_struct->sighand is still valid or not. In order to prevent a
race with __exit_sighand(), it needs to grab a
read_lock(&tasklist_lock). This strategy seems to work, since the
send_sig_info() code uses the same protection.
However, we now need to export a __send_sig_info() function, one that
expects to be called with read_lock(&tasklist_lock) already held by the
caller. It's ugly, but I doubt there is a less invasive solution.
Signed-off-by: Harald Welte <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
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
@@ -44,6 +44,7 @@
#include <linux/usb.h>
#include <linux/usbdevice_fs.h>
#include <linux/cdev.h>
+#include <linux/sched.h>
#include <asm/uaccess.h>
#include <asm/byteorder.h>
#include <linux/moduleparam.h>
@@ -231,6 +232,10 @@ static inline void async_newpending(stru
struct dev_state *ps = as->ps;
unsigned long flags;
+ /* increase refcount to task, so our as->task pointer is still
+ * valid when URB comes back -HW */
+ get_task_struct(current);
+
spin_lock_irqsave(&ps->lock, flags);
list_add_tail(&as->asynclist, &ps->async_pending);
spin_unlock_irqrestore(&ps->lock, flags);
@@ -244,8 +249,12 @@ static inline void async_removepending(s
spin_lock_irqsave(&ps->lock, flags);
list_del_init(&as->asynclist);
spin_unlock_irqrestore(&ps->lock, flags);
+
+ put_task_struct(as->task);
}
+/* get a completed URB from async list. Task reference has already been
+ * dropped in async_complete() */
static inline struct async *async_getcompleted(struct dev_state *ps)
{
unsigned long flags;
@@ -260,6 +269,7 @@ static inline struct async *async_getcom
return as;
}
+/* find matching URB from pending list. Drop refcount of task */
static inline struct async *async_getpending(struct dev_state *ps, void __user *userurb)
{
unsigned long flags;
@@ -270,6 +280,7 @@ static inline struct async *async_getpen
if (as->userurb == userurb) {
list_del_init(&as->asynclist);
spin_unlock_irqrestore(&ps->lock, flags);
+ put_task_struct(as->task);
return as;
}
spin_unlock_irqrestore(&ps->lock, flags);
@@ -290,8 +301,15 @@ 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);
+ read_lock(&tasklist_lock);
+ /* The task could be dying. We hold a reference to it,
+ * but that doesn't prevent __exit_sighand() from zeroing
+ * sighand -HW */
+ if (as->task->sighand)
+ __send_sig_info(as->signr, &sinfo, as->task);
+ read_unlock(&tasklist_lock);
}
+ put_task_struct(as->task);
wake_up(&ps->wait);
}
diff --git a/include/linux/sched.h b/include/linux/sched.h
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -999,6 +999,7 @@ extern void block_all_signals(int (*noti
sigset_t *mask);
extern void unblock_all_signals(void);
extern void release_task(struct task_struct * p);
+extern int __send_sig_info(int, struct siginfo *, struct task_struct *);
extern int send_sig_info(int, struct siginfo *, struct task_struct *);
extern int send_group_sig_info(int, struct siginfo *, struct task_struct *);
extern int force_sigsegv(int, struct task_struct *);
diff --git a/kernel/signal.c b/kernel/signal.c
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1230,12 +1230,11 @@ static int kill_something_info(int sig,
* These are for backward compatibility with the rest of the kernel source.
*/
-/*
- * These two are the most common entry points. They send a signal
- * just to the specific thread.
- */
+
+/* This is send_sig_info() for callers that already hold the tasklist_lock.
+ * At the moment the only caller is USB devfio async URB delivery. */
int
-send_sig_info(int sig, struct siginfo *info, struct task_struct *p)
+__send_sig_info(int sig, struct siginfo *info, struct task_struct *p)
{
int ret;
unsigned long flags;
@@ -1247,6 +1246,23 @@ send_sig_info(int sig, struct siginfo *i
if (!valid_signal(sig))
return -EINVAL;
+ spin_lock_irqsave(&p->sighand->siglock, flags);
+ ret = specific_send_sig_info(sig, info, p);
+ spin_unlock_irqrestore(&p->sighand->siglock, flags);
+
+ return ret;
+}
+
+/*
+ * These two are the most common entry points. They send a signal
+ * just to the specific thread.
+ */
+
+int
+send_sig_info(int sig, struct siginfo *info, struct task_struct *p)
+{
+ int ret;
+
/*
* We need the tasklist lock even for the specific
* thread case (when we don't need to follow the group
@@ -1254,9 +1270,7 @@ send_sig_info(int sig, struct siginfo *i
* going away or changing from under us.
*/
read_lock(&tasklist_lock);
- spin_lock_irqsave(&p->sighand->siglock, flags);
- ret = specific_send_sig_info(sig, info, p);
- spin_unlock_irqrestore(&p->sighand->siglock, flags);
+ ret = __send_sig_info(sig, info, p);
read_unlock(&tasklist_lock);
return ret;
}
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
iD8DBQFDNr6aXaXGVTD0i/8RAu9FAKCbbV491NzCY42YTkXo7XSzbRRO1QCgnM2K
j+Ae+NblrsPc6Jy9ICWOtyw=
=Ldn9
-----END PGP SIGNATURE-----
On Tue, Sep 27, 2005 at 05:57:55AM -0700, Greg KH wrote:
> Earlier in this thread, on these mailing lists.
>
> I've included it below too.
Ah, it was last week and I missed it. sorry.
This is more than messy. usbfs is the only user of SI_ASYNCIO, and the
way it uses it is more than messy. Why can't USB simply use the proper
AIO infrastructure?
On Tue, Sep 27, 2005 at 01:59:56PM +0100, Christoph Hellwig wrote:
> On Tue, Sep 27, 2005 at 05:57:55AM -0700, Greg KH wrote:
> > Earlier in this thread, on these mailing lists.
> >
> > I've included it below too.
>
> Ah, it was last week and I missed it. sorry.
>
> This is more than messy. usbfs is the only user of SI_ASYNCIO, and the
> way it uses it is more than messy. Why can't USB simply use the proper
> AIO infrastructure?
No one has taken the time and effort to do this. No other reason that I
know of. David? I know you have looked into this a bit in the past.
thanks,
greg k-h
On Sun, 25 Sep 2005, Harald Welte wrote:
>
> async_completed() calls send_sig_info(), which in turn does a
> spin_lock(&tasklist_lock) to protect itself from task_struct->sighand
> from going away. However, the call to
> "spin_lock_irqsave(task_struct->sighand->siglock)" causes an oops,
> because "sighand" has disappeared.
And the real bug is that you're buggering up the system in the first
place.
You don't save "current". You save "pid", and then you send a signal using
that and kill_proc_info(). End of story, bug gone. And it works with
threaded programs too, which the old thing didn't work at all with.
I refuse to apply this patch - Greg, don't even _try_ to sneak this in
through a git merge. What a horribly broken thing to do: why would USB
_ever_ need to know about things like tasklist_lock, and internal signal
handling functions and rules like "p->sighand"?
Linus
On Tue, 27 Sep 2005 at 06:09:38 Greg KH wrote:
> On Tue, Sep 27, 2005 at 01:59:56PM +0100, Christoph Hellwig wrote:
> >
> > This is more than messy. usbfs is the only user of SI_ASYNCIO, and the
> > way it uses it is more than messy. Why can't USB simply use the proper
> > AIO infrastructure?
>
> No one has taken the time and effort to do this. No other reason that I
> know of. David? I know you have looked into this a bit in the past.
Time and effort are the main issues I know of. First we'd need some
kind of FD-per-endpoint infrastructure, then it should be easy to
implement the AIO file ops, one kiocb per URB ... much like "gadgetfs"
does for USB peripherals, one kiocb per usb_request.
- Dave
On Tue, Sep 27, 2005 at 07:53:30AM -0700, Linus Torvalds wrote:
> On Sun, 25 Sep 2005, Harald Welte wrote:
> >
> > async_completed() calls send_sig_info(), which in turn does a
> > spin_lock(&tasklist_lock) to protect itself from task_struct->sighand
> > from going away. However, the call to
> > "spin_lock_irqsave(task_struct->sighand->siglock)" causes an oops,
> > because "sighand" has disappeared.
>
> And the real bug is that you're buggering up the system in the first
> place.
>
> You don't save "current". You save "pid", and then you send a signal using
> that and kill_proc_info(). End of story, bug gone. And it works with
> threaded programs too, which the old thing didn't work at all with.
And then a process calls USBDEVFS_SUBMITURB and immediately exits; its
pid gets reused by a completely different process (maybe even
root-owned), then the urb completes, and kill_proc_info() sends the
signal to the unsuspecting process.
> I refuse to apply this patch - Greg, don't even _try_ to sneak this in
> through a git merge. What a horribly broken thing to do: why would USB
> _ever_ need to know about things like tasklist_lock, and internal signal
> handling functions and rules like "p->sighand"?
Hmm, then probably send_sig_info() should check for non-NULL
p->sighand after taking tasklist_lock? Otherwise all uses of
send_sig_info() for non-current tasks are unsafe.
"git grep -w send_sig_info" shows that most callers call
send_sig_info() for the current task, except these:
arch/sparc/kernel/traps.c: send_sig_info(SIGFPE, &info, fpt);
arch/sparc64/kernel/sys_sunos32.c: send_sig_info(SIGSYS, &info, current);
drivers/char/drm/drm_irq.c: send_sig_info( vbl_sig->info.si_signo, &vbl_sig->info, vbl_sig->task );
drivers/usb/core/devio.c: send_sig_info(as->signr, &sinfo, as->task);
drivers/usb/core/inode.c: send_sig_info(ds->discsignr, &sinfo, ds->disctask);
drivers/usb/gadget/file_storage.c: send_sig_info(SIGUSR1, SEND_SIG_FORCED, thread_task);
kernel/signal.c: return send_sig_info(sig, (void*)(long)(priv != 0), p);
BTW, all other callers of send_sig_info() are under
arch/{alpha,arm,sparc,sparc64}/ - 23 total.
On Tue, 27 Sep 2005, Sergey Vlasov wrote:
>
> And then a process calls USBDEVFS_SUBMITURB and immediately exits; its
> pid gets reused by a completely different process (maybe even
> root-owned), then the urb completes, and kill_proc_info() sends the
> signal to the unsuspecting process.
Ehh.. pid's don't get re-used until they wrap.
Your _current_ code has that problem, though - "struct task_struct" _does_
get re-used.
Don't assume that the fixes are as bad.
Anyway, Christoph is certainly correct that what you _should_ be using is
the SIGIO infrastructure, even if you don't actually use the fcntl() to
register it.
> Hmm, then probably send_sig_info() should check for non-NULL
> p->sighand after taking tasklist_lock? Otherwise all uses of
> send_sig_info() for non-current tasks are unsafe.
I don't think so.
Your oops is because you're using a STALE POINTER.
If you look it up by pid, it won't be stale, now will it?
Hint: the point where sighand is released is also the point where the
process is unhashed.
Linus
On Maw, 2005-09-27 at 09:09 -0700, Linus Torvalds wrote:
> > root-owned), then the urb completes, and kill_proc_info() sends the
> > signal to the unsuspecting process.
>
> Ehh.. pid's don't get re-used until they wrap.
Which doesn't take very long to arrange. Relying on pids is definitely a
security problem we don't want to make worse than it already is.
> If you look it up by pid, it won't be stale, now will it?
Just potentially wrong, but if it uses the SIGIO code and the SIGIO code
is fixed then it works out.
On Tue, Sep 27, 2005 at 09:09:28AM -0700, Linus Torvalds wrote:
> On Tue, 27 Sep 2005, Sergey Vlasov wrote:
> >
> > And then a process calls USBDEVFS_SUBMITURB and immediately exits; its
> > pid gets reused by a completely different process (maybe even
> > root-owned), then the urb completes, and kill_proc_info() sends the
> > signal to the unsuspecting process.
>
> Ehh.. pid's don't get re-used until they wrap.
#define PID_MAX_DEFAULT (CONFIG_BASE_SMALL ? 0x1000 : 0x8000)
which is not that big (and on 32-bit systems PID_MAX_LIMIT is also
32K).
> Your _current_ code has that problem, though - "struct task_struct" _does_
> get re-used.
The initial patch added get_task_struct()/put_task_struct() calls to
fix this - are they forbidden too?
> Don't assume that the fixes are as bad.
>
> Anyway, Christoph is certainly correct that what you _should_ be using is
> the SIGIO infrastructure, even if you don't actually use the fcntl() to
> register it.
It at least has sigio_perm(), which prevents exploiting it to send
signals to tasks you don't have access to.
> > Hmm, then probably send_sig_info() should check for non-NULL
> > p->sighand after taking tasklist_lock? Otherwise all uses of
> > send_sig_info() for non-current tasks are unsafe.
>
> I don't think so.
>
> Your oops is because you're using a STALE POINTER.
>
> If you look it up by pid, it won't be stale, now will it?
>
> Hint: the point where sighand is released is also the point where the
> process is unhashed.
When using kill_proc_info() - yes, because it takes tasklist_lock
during its operation. However, lookup by pid is not safe against pid
wrapping (at least without adding more checks like sigio_perm(), which
ensure that you can zap only your own process with that signal).
(Why they did not make a kind of "file descriptor" for processes...)
On Tue, 27 Sep 2005, Alan Cox wrote:
>
> Which doesn't take very long to arrange. Relying on pids is definitely a
> security problem we don't want to make worse than it already is.
The thing is, the current code is _worse_.
MUCH worse.
And it's worse exactly because it does things really wrong. The suggested
patch then just _continues_ to do things really wrong, and then tries to
paper over the bugs.
Which is why I refuse to apply it. Use a pid and do it right.
If the code cannot be made to use fasync itself, then it can at least be
made to do the same _checks_ that fasync does (easy enough: just save away
uid/euid, and do the same signal checks by hand). Until such a time than
the driver writer sees the light.
Linus
On Tue, 27 Sep 2005, Sergey Vlasov wrote:
>
> The initial patch added get_task_struct()/put_task_struct() calls to
> fix this - are they forbidden too?
They are sure as hell not something that a _driver_ is supposed to use.
> It at least has sigio_perm(), which prevents exploiting it to send
> signals to tasks you don't have access to.
And the point is, you can do that _too_.
Do it right. Don't cache pointers to threads. Use the pid.
Your security arguments are _pointless_. As proven by the fact that SIGIO
happily uses a pid, and gets it right. Try to use _that_ infrastructure
instead, since that's what it's _meant_ for.
The fact is, having drivers much around with thread locking is not
acceptable. Drivers _will_ get it wrong, and even if they didn't, it's
kernel internal data structures that drivers have no business in touching.
Linus
[ I am changing the topic somewhat, so I've trimmed the CC list and
adjusted the Subject. ]
On Tue, Sep 27, 2005 at 08:52:06PM +0400, Sergey Vlasov wrote:
> (Why they did not make a kind of "file descriptor" for processes...)
Actually, I made a proposal back in 1999 which I think would let many
userspace apps deal with PID reuse nicely.
The idea is to introduce a kernel call (it can be a prctl(2) setting,
although my pseudo-code "defines" an entire syscall for simplicity)
which would "lock" the invoking process' view of a given PID (while
letting the PID get reused - so there's no added risk of DoS). The
original posting and subsequent thread can be seen here:
http://lists.nas.nasa.gov/archives/ext/linux-security-audit/1999/08/msg00108.html
The proposal itself (unedited since 1999, but the idea holds) is as
follows:
in task_struct:
int locked_pid;
int sys_lockpid(int pid)
{
int old;
old = current->locked_pid;
current->locked_pid = pid;
return old;
}
on kill(2) and ptrace(2):
if (pid > 0 && -pid == current->locked_pid)
return -ESRCH;
on execve(2):
current->locked_pid = 0;
on fork(2), in get_pid(), where last_pid is the PID being allocated:
for_each_task (p)
if (p->locked_pid == last_pid) p->locked_pid = -lastpid;
in applications, such as killall(1):
do {
lockpid(target);
if (!need_to_kill(target)) break;
if (kill(target, SIGKILL) == 0) break;
} while (errno == ESRCH);
lockpid(0);
Performance can be improved by maintaining a global locked_pid_count,
so that fork(2) could skip the loop if count is zero. Implementing
this would require an extra spinlock (the pseudo-code above will need
some anyway, if actually implemented).
It is possible to clear locked_pid in kill(2) and ptrace(2), but I'm
not sure whether that's a good idea, as we could have these syscalls
in signal handlers that are not aware of the new feature.
--
Alexander
On Maw, 2005-09-27 at 21:20 +0400, Solar Designer wrote:
> The idea is to introduce a kernel call (it can be a prctl(2) setting,
> although my pseudo-code "defines" an entire syscall for simplicity)
> which would "lock" the invoking process' view of a given PID (while
> letting the PID get reused - so there's no added risk of DoS). The
> original posting and subsequent thread can be seen here:
You can solve it just as well in kernel space without application
changes. Given a refcounted structure something like
struct pidref {
atomic_t ref;
struct pidref *next, *prev;
pid_t pid;
};
and a hash you can take a pid reference whenever you hang onto a pid in
kernel space and check what should be a tiny if not empty hash in the
normal cases whenever you allocate a pid.
Alan
On Maw, 2005-09-27 at 09:59 -0700, Linus Torvalds wrote:
>
> On Tue, 27 Sep 2005, Alan Cox wrote:
> >
> > Which doesn't take very long to arrange. Relying on pids is definitely a
> > security problem we don't want to make worse than it already is.
>
> The thing is, the current code is _worse_.
>
> MUCH worse.
Violent agreement
On Tue, 27 Sep 2005, Alan Cox wrote:
>
> On Maw, 2005-09-27 at 21:20 +0400, Solar Designer wrote:
> > The idea is to introduce a kernel call (it can be a prctl(2) setting,
> > although my pseudo-code "defines" an entire syscall for simplicity)
> > which would "lock" the invoking process' view of a given PID (while
> > letting the PID get reused - so there's no added risk of DoS). The
> > original posting and subsequent thread can be seen here:
>
> You can solve it just as well in kernel space without application
> changes.
Note that for at least signal sending, the security aspect is _not_ about
whether the pid has been re-used, but about whether the _user_ matches.
This is most trivially seen by thinking about just a suid exec.
Sending a signal to a suid process should be disallowed even _if_ the
"struct task_struct" is still the same, which is why the oops fix
discussed in this thread is totally pointless - it has almost nothing to
do with security. Doing "get_task_struct()" may remove the oops, but it
doesn't remove the security issues (pid wrapping is a total red herring:
with get_task_struct you don't need to wrap pids at all, you just do a
single exec on a suid executable, and off you go).
Now, the fasync interfaces are a bit inconvenient because they require the
"struct file" to be around, since that's what also contains the owner
information. That means that you have to track file lifetime in the urb
submission.
I don't know the code, but if that's inconvenient, then the real solution
ends up being to just do the permission checks by hand. Remember the
uid/euid/pid at the time of the submission, and use them at completion.
I don't even think it's worth any general helper infrastructure: I suspect
the interfaces are pretty broken as designed, and we should _not_
encourage them further. But it's not like the security check is more than
three lines of code, so..
Linus
Perhaps I should have dropped the old Subject completely, despite the
topic I brought up being related to what was discussed. I am talking
about PID reuse related issues in userspace apps that appear to require
an additional kernel interface to be resolved.
On Tue, Sep 27, 2005 at 09:34:11PM +0100, Alan Cox wrote:
> On Maw, 2005-09-27 at 21:20 +0400, Solar Designer wrote:
> > The idea is to introduce a kernel call (it can be a prctl(2) setting,
> > although my pseudo-code "defines" an entire syscall for simplicity)
> > which would "lock" the invoking process' view of a given PID (while
> > letting the PID get reused - so there's no added risk of DoS).
>
> You can solve it just as well in kernel space without application
> changes.
Not really. For example, the kernel currently has no idea, and no
reliable way to find out, that killall(1) has taken note of a PID and is
about to kill it.
> Given a refcounted structure something like
>
> struct pidref {
> atomic_t ref;
> struct pidref *next, *prev;
> pid_t pid;
> };
>
> and a hash you can take a pid reference whenever you hang onto a pid in
> kernel space and check what should be a tiny if not empty hash in the
> normal cases whenever you allocate a pid.
That would work for certain in-kernel issues.
If we would let userspace apps lock PIDs like that, there would be added
DoS risk that would need to be dealt with (e.g., by letting a process
lock only one other PID - provided that the PID space is sufficiently
large that having a half of it locked is not a problem). But I do not
see a reason to go for that complexity. My proposal avoids it.
(A hash table could be used along with my proposal as well to speed
things up, but it would not affect _allocation_ of PIDs. Rather, only
the "locking" process's view would be affected by the "lock".)
--
Alexander
On Tue, Sep 27, 2005 at 01:42:44PM -0700, Linus Torvalds wrote:
> Note that for at least signal sending, the security aspect is _not_ about
> whether the pid has been re-used, but about whether the _user_ matches.
That's true. And, changing topic to userspace apps, killall(1)
currently has no race-free way to check whether the user still matches.
There's also the reliability aspect: killing one's own process, but
other than the intended one, is a reliability issue.
What I have proposed is a way to deal with both of these.
killall is just an example. A GUI point-and-click task manager would
have the same problem and the same solution would work for it.
--
Alexander
On Tue, Sep 27, 2005 at 10:02:16AM -0700, Linus Torvalds wrote:
> On Tue, 27 Sep 2005, Sergey Vlasov wrote:
> >
> > The initial patch added get_task_struct()/put_task_struct() calls to
> > fix this - are they forbidden too?
>
> They are sure as hell not something that a _driver_ is supposed to use.
>
> > It at least has sigio_perm(), which prevents exploiting it to send
> > signals to tasks you don't have access to.
>
> And the point is, you can do that _too_.
>
> Do it right. Don't cache pointers to threads. Use the pid.
So what happened to this thread? Was I simply removed from the Cc, or
did the thread cease? Or is there a "secret" fix on vendor-sec?
I'm probably the person in this thread who understands the least about
the USB stack and the scheduler, but if there is no implementation of
Linus' suggested "PID approach" yet, I'd be willing to write a patch and
test it. Please let me know.
Please also understand that I never argued that my initial patch was a
good solution, or that I wanted it to have merged. I just wanted to
show that there is a real-world problem, and it somehow needs to be fixed.
Cheers,
--
- Harald Welte <[email protected]> http://gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
On Fri, 30 Sep 2005, Harald Welte wrote:
>
> I'm probably the person in this thread who understands the least about
> the USB stack and the scheduler, but if there is no implementation of
> Linus' suggested "PID approach" yet, I'd be willing to write a patch and
> test it. Please let me know.
Here's a totally untested patch. It's guaranteed not to do the "right
thing", simply because it doesn't _use_ the uid/euid information. But it's
in the right kind of direction.
If you change the "kill_proc_info()" into a "kill_proc_info_as_uid()"
call, and add that to kernel/signal.c (which is basically kill_proc_info()
except it uses the passed-in uid/euid for the "check_kill_permission()"
tests instead), it should be correct.
As-is, it won't work, because it will use a _random_ uid (whatever is the
currently running process) for the kill permission. So this really is just
a "use this as a template" kind of patch, DO NOT APPLY!
Linus
---
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
@@ -58,7 +58,8 @@ static struct class *usb_device_class;
struct async {
struct list_head asynclist;
struct dev_state *ps;
- struct task_struct *task;
+ pid_t pid;
+ uid_t uid, euid;
unsigned int signr;
unsigned int ifnum;
void __user *userbuffer;
@@ -290,7 +291,14 @@ 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);
+ /*
+ * We should have a
+ *
+ * kill_proc_info_as_uid(signr, info, pid, uid, euid);
+ *
+ * but we don't.
+ */
+ kill_proc_info(as->signr, &sinfo, as->pid);
}
wake_up(&ps->wait);
}
@@ -988,7 +996,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);
* Linus Torvalds ([email protected]) wrote:
> Here's a totally untested patch. It's guaranteed not to do the "right
> thing", simply because it doesn't _use_ the uid/euid information. But it's
> in the right kind of direction.
>
> If you change the "kill_proc_info()" into a "kill_proc_info_as_uid()"
> call, and add that to kernel/signal.c (which is basically kill_proc_info()
> except it uses the passed-in uid/euid for the "check_kill_permission()"
> tests instead), it should be correct.
>
> As-is, it won't work, because it will use a _random_ uid (whatever is the
> currently running process) for the kill permission. So this really is just
> a "use this as a template" kind of patch, DO NOT APPLY!
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. Is there a reason not to do normal async here?
thanks,
-chris
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.
Linus
* Linus Torvalds ([email protected]) wrote:
> 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.
Ah, I see.
thanks,
-chris
On Sat, 1 Oct 2005, Harald Welte wrote:
>
> 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.
No, you can't re-use "check_kill_permissions()" like this, even though I
do understand the appeal.
The generic kill permissions check things like the current session, and
whether the caller has extra permissions, neither of which is sensible in
the context of "group_send_sig_info_as_uid()". So you do need to write out
the uid/euid checks separately, and have a different function for this
thing.
Linus
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 <[email protected]> http://gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
On Fri, Sep 30, 2005 at 03:16:28PM -0700, Linus Torvalds wrote:
>
>
> On Sat, 1 Oct 2005, Harald Welte wrote:
> >
> > 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.
>
> No, you can't re-use "check_kill_permissions()" like this, even though I
> do understand the appeal.
>
> The generic kill permissions check things like the current session, and
> whether the caller has extra permissions, neither of which is sensible in
> the context of "group_send_sig_info_as_uid()". So you do need to write out
> the uid/euid checks separately, and have a different function for this
> thing.
Sorry, I've been busy, mostly with the annual netfilter developer
workshop. What about the following proposed fix:
[USBDEVIO] Fix Oops in usbdevio async URB completion (CAN-2005-3055)
If a process issues an URB from userspace and (starts to) terminate
before the URB comes back, we run into the issue described above. This
is because the urb saves a pointer to "current" when it is posted to the
device, but there's no guarantee that this pointer is still valid
afterwards.
This basically means that any userspace process with permissions to
any arbitrary USB device can Ooops any kernel.(!)
In fact, there are two separate issues:
1) the pointer to "current" can become invalid, since the task could be
completely gone when the URB completion comes back from the device.
2) Even if the saved task pointer is still pointing to a valid task_struct,
task_struct->sighand could have gone meanwhile. Therefore, the USB
async URB completion handler needs to reliably check whether
task_struct->sighand is still valid or not. In order to prevent a
race with __exit_sighand(), it needs to grab a
read_lock(&tasklist_lock). This strategy seems to work, since the
send_sig_info() code uses the same protection.
However, we now would need to export a __send_sig_info() function, one
that expects to be called with read_lock(&tasklist_lock) already held by
the caller.
So what we do instead, is to save the PID of the process, and introduce a
new kill_proc_info_as_uid() function. Yes, pid's can and will wrap, so
this is just a short-term fix until usbfs2 will appear.
Signed-off-by: Harald Welte <[email protected]>
---
commit c630b8abcd6aa848c2b2b2e546820fa0a7dd0736
tree 0b48db1d918299cc07311b01b2a2dbc8c592a852
parent e759eaa9e9e92330c5fcfd760d767d4f39375a03
author Harald Welte <[email protected]> Mon, 10 Oct 2005 19:42:46 +0200
committer Harald Welte <[email protected]> Mon, 10 Oct 2005 19:42:46 +0200
drivers/usb/core/devio.c | 12 +++++++++---
include/linux/sched.h | 1 +
kernel/signal.c | 34 ++++++++++++++++++++++++++++++++++
3 files changed, 44 insertions(+), 3 deletions(-)
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
@@ -1193,6 +1193,40 @@ 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 ret = -EINVAL;
+ struct task_struct *p;
+
+ if (!valid_signal(sig))
+ return ret;
+
+ read_lock(&tasklist_lock);
+ p = find_task_by_pid(pid);
+ if (!p) {
+ ret = -ESRCH;
+ goto out_unlock;
+ }
+ if ((!info || ((unsigned long)info != 1 &&
+ (unsigned long)info != 2 && SI_FROMUSER(info)))
+ && (euid ^ p->suid) && (euid ^ p->uid)
+ && (uid ^ p->suid) && (uid ^ p->uid)) {
+ ret = -EPERM;
+ goto out_unlock;
+ }
+ if (sig && p->sighand) {
+ unsigned long flags;
+ spin_lock_irqsave(&p->sighand->siglock, flags);
+ ret = __group_send_sig_info(sig, info, p);
+ spin_unlock_irqrestore(&p->sighand->siglock, flags);
+ }
+out_unlock:
+ read_unlock(&tasklist_lock);
+ return ret;
+}
/*
* kill_something_info() interprets pid in interesting ways just like kill(2).
--
- Harald Welte <[email protected]> http://gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
* Harald Welte ([email protected]) wrote:
> diff --git a/kernel/signal.c b/kernel/signal.c
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1193,6 +1193,40 @@ kill_proc_info(int sig, struct siginfo *
> return error;
> }
>
> +/* like kill_proc_info(), but doesn't use uid/euid of "current" */
Maybe additional comment reminding that you most likely don't want this
interface.
Also, looks like there's same issue again:
drivers/usb/core/devio.c::usbdev_open()
...
ps->disctask = current;
drivers/usb/core/inode.c::usbfs_remove_device()
...
send_sig_info(ds->discsignr, &sinfo, ds->disctask);
On Mon, 10 Oct 2005, Harald Welte wrote:
>
> Sorry, I've been busy, mostly with the annual netfilter developer
> workshop. What about the following proposed fix:
Yes, looks ok, apart from some small details. Like "uid" adn "euid" is of
type "uid_t", not "pid_t", and I think that "kill_proc_info_as_uid()"
needs exporting for modules (I assume usbdevio can be one).
Chris, others, comments?
Linus
On Mon, 10 Oct 2005, Harald Welte wrote:
> + if ((!info || ((unsigned long)info != 1 &&
> + (unsigned long)info != 2 && SI_FROMUSER(info)))
> + && (euid ^ p->suid) && (euid ^ p->uid)
> + && (uid ^ p->suid) && (uid ^ p->uid)) {
No doubt this was copied from somewhere else. But why do people go to the
effort of confusing readers by using "^" instead of "!="? These aren't
bit-oriented values.
Alan Stern
* Linus Torvalds ([email protected]) wrote:
> Yes, looks ok, apart from some small details. Like "uid" adn "euid" is of
> type "uid_t", not "pid_t", and I think that "kill_proc_info_as_uid()"
> needs exporting for modules (I assume usbdevio can be one).
>
> Chris, others, comments?
Agree with above, but I hope it could be considered a temporary interface.
thanks,
-chris
On Mon, Oct 10, 2005 at 04:03:13PM -0400, Alan Stern wrote:
> On Mon, 10 Oct 2005, Harald Welte wrote:
>
> > + if ((!info || ((unsigned long)info != 1 &&
> > + (unsigned long)info != 2 && SI_FROMUSER(info)))
> > + && (euid ^ p->suid) && (euid ^ p->uid)
> > + && (uid ^ p->suid) && (uid ^ p->uid)) {
>
> No doubt this was copied from somewhere else. But why do people go to the
> effort of confusing readers by using "^" instead of "!="? These aren't
> bit-oriented values.
Well, I'd rather keep the new code as close as possible to the original
check_permission() code, to make it obvious that it's basically doing
the same thing. I think if you want to clean this up, it could be an
additional patch on top of mine (once there is a final version and it
gets merged.
> Alan Stern
--
- Harald Welte <[email protected]> http://gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
On Mon, Oct 10, 2005 at 11:07:45AM -0700, Chris Wright wrote:
> * Harald Welte ([email protected]) wrote:
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -1193,6 +1193,40 @@ kill_proc_info(int sig, struct siginfo *
> > return error;
> > }
> >
> > +/* like kill_proc_info(), but doesn't use uid/euid of "current" */
>
> Maybe additional comment reminding that you most likely don't want this
> interface.
>
> Also, looks like there's same issue again:
Mh, didn't hit that bug since I don't use disconnect signals. But it
looks like it has the same issue.
Please consider the patch below, it
1) changes pid_t to uid_t
2) exports the symbol
3) adresses the same task_struct referencing issue for disconnect
signals
I hope this now finally is the last take ;)
[USBDEVIO] Fix Oops in usbdevio async URB completion (CAN-2005-3055)
If a process issues an URB from userspace and (starts to) terminate
before the URB comes back, we run into the issue described above. This
is because the urb saves a pointer to "current" when it is posted to the
device, but there's no guarantee that this pointer is still valid
afterwards.
This basically means that any userspace process with permissions to
any arbitrary USB device can Ooops any kernel.(!)
In fact, there are two separate issues:
1) the pointer to "current" can become invalid, since the task could be
completely gone when the URB completion comes back from the device.
2) Even if the saved task pointer is still pointing to a valid task_struct,
task_struct->sighand could have gone meanwhile. Therefore, the USB
async URB completion handler needs to reliably check whether
task_struct->sighand is still valid or not. In order to prevent a
race with __exit_sighand(), it needs to grab a
read_lock(&tasklist_lock). This strategy seems to work, since the
send_sig_info() code uses the same protection.
However, we now would need to export a __send_sig_info() function, one
that expects to be called with read_lock(&tasklist_lock) already held by
the caller.
So what we do instead, is to save the PID of the process, and introduce a
new kill_proc_info_as_uid() function. Yes, pid's can and will wrap, so
this is just a short-term fix until usbfs2 will appear.
Signed-off-by: Harald Welte <[email protected]>
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;
+ uid_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);
}
@@ -525,9 +529,11 @@ static int usbdev_open(struct inode *ino
INIT_LIST_HEAD(&ps->async_pending);
INIT_LIST_HEAD(&ps->async_completed);
init_waitqueue_head(&ps->wait);
- ps->discsignr = 0;
- ps->disctask = current;
- ps->disccontext = NULL;
+ ps->disc.signr = 0;
+ ps->disc.pid = current->pid;
+ ps->disc.uid = current->uid;
+ ps->disc.euid = current->euid;
+ ps->disc.context = NULL;
ps->ifclaimed = 0;
wmb();
list_add_tail(&ps->list, &dev->filelist);
@@ -988,7 +994,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);
@@ -1203,8 +1211,8 @@ static int proc_disconnectsignal(struct
return -EFAULT;
if (ds.signr != 0 && (ds.signr < SIGRTMIN || ds.signr > SIGRTMAX))
return -EINVAL;
- ps->discsignr = ds.signr;
- ps->disccontext = ds.context;
+ ps->disc.signr = ds.signr;
+ ps->disc.context = ds.context;
return 0;
}
diff --git a/drivers/usb/core/inode.c b/drivers/usb/core/inode.c
--- a/drivers/usb/core/inode.c
+++ b/drivers/usb/core/inode.c
@@ -708,12 +708,14 @@ void usbfs_remove_device(struct usb_devi
ds = list_entry(dev->filelist.next, struct dev_state, list);
wake_up_all(&ds->wait);
list_del_init(&ds->list);
- if (ds->discsignr) {
+ if (ds->disc.signr) {
sinfo.si_signo = SIGPIPE;
sinfo.si_errno = EPIPE;
sinfo.si_code = SI_ASYNCIO;
- sinfo.si_addr = ds->disccontext;
- send_sig_info(ds->discsignr, &sinfo, ds->disctask);
+ sinfo.si_addr = ds->disc.context;
+ kill_proc_info_as_uid(ds->disc.signr, &sinfo,
+ ds->disc.pid, ds->disc.uid,
+ ds->disc.euid);
}
}
usbfs_update_special();
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -51,9 +51,12 @@ struct dev_state {
struct list_head async_pending;
struct list_head async_completed;
wait_queue_head_t wait; /* wake up if a request completed */
- unsigned int discsignr;
- struct task_struct *disctask;
- void __user *disccontext;
+ struct {
+ unsigned int signr;
+ pid_t pid;
+ uid_t uid, euid;
+ void __user *context;
+ } disc;
unsigned long ifclaimed;
};
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, uid_t, uid_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
@@ -1193,6 +1193,42 @@ kill_proc_info(int sig, struct siginfo *
return error;
}
+/* This is like kill_proc_info(), but doesn't use uid/euid of "current".
+ * Most likely this function is not what you want, it was added as a special
+ * case for usbdevio */
+int
+kill_proc_info_as_uid(int sig, struct siginfo *info, pid_t pid,
+ uid_t uid, uid_t euid)
+{
+ int ret = -EINVAL;
+ struct task_struct *p;
+
+ if (!valid_signal(sig))
+ return ret;
+
+ read_lock(&tasklist_lock);
+ p = find_task_by_pid(pid);
+ if (!p) {
+ ret = -ESRCH;
+ goto out_unlock;
+ }
+ if ((!info || ((unsigned long)info != 1 &&
+ (unsigned long)info != 2 && SI_FROMUSER(info)))
+ && (euid ^ p->suid) && (euid ^ p->uid)
+ && (uid ^ p->suid) && (uid ^ p->uid)) {
+ ret = -EPERM;
+ goto out_unlock;
+ }
+ if (sig && p->sighand) {
+ unsigned long flags;
+ spin_lock_irqsave(&p->sighand->siglock, flags);
+ ret = __group_send_sig_info(sig, info, p);
+ spin_unlock_irqrestore(&p->sighand->siglock, flags);
+ }
+out_unlock:
+ read_unlock(&tasklist_lock);
+ return ret;
+}
/*
* kill_something_info() interprets pid in interesting ways just like kill(2).
@@ -1974,6 +2010,7 @@ EXPORT_SYMBOL(flush_signals);
EXPORT_SYMBOL(force_sig);
EXPORT_SYMBOL(kill_pg);
EXPORT_SYMBOL(kill_proc);
+EXPORT_SYMBOL_GPL(kill_proc_info_as_uid);
EXPORT_SYMBOL(ptrace_notify);
EXPORT_SYMBOL(send_sig);
EXPORT_SYMBOL(send_sig_info);
--
- Harald Welte <[email protected]> http://gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
On Mon, 2005-10-10 at 11:07 -0700, Chris Wright wrote:
> * Harald Welte ([email protected]) wrote:
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -1193,6 +1193,40 @@ kill_proc_info(int sig, struct siginfo *
> > return error;
> > }
> >
> > +/* like kill_proc_info(), but doesn't use uid/euid of "current" */
>
> Maybe additional comment reminding that you most likely don't want this
> interface.
Just mark it DEPRECATED since it basically is that?!
Bernd
--
Firmix Software GmbH http://www.firmix.at/
mobil: +43 664 4416156 fax: +43 1 7890849-55
Embedded Linux Development and Services
Alan asked:
> But why do people go to the
> effort of confusing readers by using "^" instead of "!="?
My guess - eor (^) was quicker than not-equal (!=) on a PDP-11.
That code fragment for checking uid's has been around a -long-
time, if my memory serves me.
It's gotten to be like the infamous "!!" boolean conversion
operator, a bit of vernacular that would be harder to read if
recoded using modern coding style.
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.925.600.0401
On Tue, 11 Oct 2005, Paul Jackson wrote:
> Alan asked:
>> But why do people go to the
>> effort of confusing readers by using "^" instead of "!="?
>
> My guess - eor (^) was quicker than not-equal (!=) on a PDP-11.
>
> That code fragment for checking uid's has been around a -long-
> time, if my memory serves me.
>
> It's gotten to be like the infamous "!!" boolean conversion
> operator, a bit of vernacular that would be harder to read if
> recoded using modern coding style.
>
> --
> I won't rest till it's the best ...
> Programmer, Linux Scalability
> Paul Jackson <[email protected]> 1.925.600.0401
Also, at one time, people used to spend a lot of time
minimizing the number of CPU cycles used in the code.
For instance, when it's appropriate, using XOR makes the
resulting generated code simpler and usually faster:
int funct0(int i)
{
return i ^ 1;
}
int funct1(int i)
{
return (i != 1);
}
int main()
{
int i;
for(i=0; i< 100; i++)
{
if(funct0(i))
printf("Yep %d\n", i);
if(funct1(i))
printf("Yep %d\n", i);
}
return 0;
}
gcc -O2 -fomit-frame-pointer ...
Here, funct0 clearly uses less code.
Disassembly of section .text:
00000000 <funct0>:
0: 8b 44 24 04 mov 0x4(%esp),%eax
4: 83 f0 01 xor $0x1,%eax
7: c3 ret
00000008 <funct1>:
8: 31 c0 xor %eax,%eax
a: 83 7c 24 04 01 cmpl $0x1,0x4(%esp)
f: 0f 95 c0 setne %al
12: c3 ret
13: 90 nop
00000014 <main>:
[SNIPPED...]
Cheers,
Dick Johnson
Penguin : Linux version 2.6.13 on an i686 machine (5589.44 BogoMips).
Warning : 98.36% of all statistics are fiction.
.
****************************************************************
The information transmitted in this message is confidential and may be privileged. Any review, retransmission, dissemination, or other use of this information by persons or entities other than the intended recipient is prohibited. If you are not the intended recipient, please notify Analogic Corporation immediately - by replying to this message or by sending an email to [email protected] - and destroy all copies of this information, including any attachments, without reading or disclosing them.
Thank you.
On Tue, 11 Oct 2005, linux-os (Dick Johnson) wrote:
> On Tue, 11 Oct 2005, Paul Jackson wrote:
>
> > Alan asked:
> >> But why do people go to the
> >> effort of confusing readers by using "^" instead of "!="?
> >
> > My guess - eor (^) was quicker than not-equal (!=) on a PDP-11.
> >
> > That code fragment for checking uid's has been around a -long-
> > time, if my memory serves me.
> >
> > It's gotten to be like the infamous "!!" boolean conversion
> > operator, a bit of vernacular that would be harder to read if
> > recoded using modern coding style.
Surely Linux uses entirely original code, with no hangovers from the
original AT&T Unix... Besides, to the best of my recollection, the two
operations are equal in speed on a PDP-11.
"!!" makes sense as an idiom. But "^" for "!=" doesn't, at least not in
this context.
> Also, at one time, people used to spend a lot of time
> minimizing the number of CPU cycles used in the code.
>
> For instance, when it's appropriate, using XOR makes the
> resulting generated code simpler and usually faster:
...
Yes, sometimes XOR can yield simpler object code. But not in cases like
this, where it's part of a Boolean test:
if (... && (a1^b1) && (a2^b2) && (a3^b3)) ...
On any architecture I know of, "^" and "!=" would be equally efficient
here.
Alan Stern
On Tue, Oct 11, 2005 at 03:13:39PM -0400, Alan Stern wrote:
> Surely Linux uses entirely original code, with no hangovers from the
> original AT&T Unix... Besides, to the best of my recollection, the two
> operations are equal in speed on a PDP-11.
I've no idea but I don't believe the relative speed of PDP_11 operations is
security critical in Linux so can you trim the cc line
On Tue, Oct 11, 2005 at 11:45:51AM +0200, Harald Welte wrote:
> On Mon, Oct 10, 2005 at 11:07:45AM -0700, Chris Wright wrote:
> > * Harald Welte ([email protected]) wrote:
> > > diff --git a/kernel/signal.c b/kernel/signal.c
> > > --- a/kernel/signal.c
> > > +++ b/kernel/signal.c
> > > @@ -1193,6 +1193,40 @@ kill_proc_info(int sig, struct siginfo *
> > > return error;
> > > }
> > >
> > > +/* like kill_proc_info(), but doesn't use uid/euid of "current" */
> >
> > Maybe additional comment reminding that you most likely don't want this
> > interface.
> >
> > Also, looks like there's same issue again:
>
> Mh, didn't hit that bug since I don't use disconnect signals. But it
> looks like it has the same issue.
>
> Please consider the patch below, it
>
> 1) changes pid_t to uid_t
> 2) exports the symbol
> 3) adresses the same task_struct referencing issue for disconnect
> signals
>
> I hope this now finally is the last take ;)
Ugh, but it looks like Linus already committed your previous patch, with
some changes by him. Care to send a delta from what is currently in his
tree (2.6.14-rc4 has it) and this patch?
thanks,
greg k-h
On Tue, 11 Oct 2005, Greg KH wrote:
>
> Ugh, but it looks like Linus already committed your previous patch, with
> some changes by him. Care to send a delta from what is currently in his
> tree (2.6.14-rc4 has it) and this patch?
I _think_ I fixed the disconnect thing too, although I think Harald's
naming for the disconnect structure was cleaner, so I wouldn't mind having
a (tested) patch on top of mine..
To some degree it would actually be nice to totally abstract that
"pid+uid+euid" thing out as a structure of its own, and have the signal
handling code fill it up (helper inline function in <linux/sched.h> or
something), and have the users just use what to them is a totally opaque
"signal sender token".
That would allow us to improve or change the validation of the thing
later.
But for 2.6.14, the most important thing would be to verify that the oops
cannot happen, and that you can't send signals to setuid programs by doing
an "open(usb) + fork(keep it open in the child) + exec(suid in the
parent)"
Linus
On Tue, Oct 11, 2005 at 04:44:41PM -0700, Linus Torvalds wrote:
> I _think_ I fixed the disconnect thing too, although I think Harald's
> naming for the disconnect structure was cleaner, so I wouldn't mind having
> a (tested) patch on top of mine..
> [...]
> But for 2.6.14, the most important thing would be to verify that the oops
> cannot happen, and that you can't send signals to setuid programs by doing
> an "open(usb) + fork(keep it open in the child) + exec(suid in the
> parent)"
I'm busy giving a netfilter tutorial all day, but I'll do the
incremental patch + testing tonight. So I expect some kind of reply on
this at about 11pm (GMT+1).
Sorry once again for the delays, but as I said initially: I'm neither a
core kernel / scheduler nor a USB developer. If somebody else had taken
the bug, we'd have had way less round-trips...
Cheers,
--
- Harald Welte <[email protected]> http://gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
On Tue, Oct 11, 2005 at 11:45:51AM +0200, Harald Welte wrote:
> On Mon, Oct 10, 2005 at 11:07:45AM -0700, Chris Wright wrote:
> > * Harald Welte ([email protected]) wrote:
> > > diff --git a/kernel/signal.c b/kernel/signal.c
> > > --- a/kernel/signal.c
> > > +++ b/kernel/signal.c
> > > @@ -1193,6 +1193,40 @@ kill_proc_info(int sig, struct siginfo *
> > > return error;
> > > }
> > >
> > > +/* like kill_proc_info(), but doesn't use uid/euid of "current" */
> >
> > Maybe additional comment reminding that you most likely don't want this
> > interface.
> >
> > Also, looks like there's same issue again:
I know this patch is going to get rediffed before being applied to
Linus's tree, but in case anyone cares, here is a version that hasn't
been mangled - all the copies I found on online archives seemed to
have some encoding junk in there.
I did this by hand, so an error may have crept in.
It seems to apply cleanly to 2.6.13 (and 2.6.12).
I haven't done any additional testing,
like say trying to compile it.
--
Horms
Mh, didn't hit that bug since I don't use disconnect signals. But it
looks like it has the same issue.
Please consider the patch below, it
1) changes pid_t to uid_t
2) exports the symbol
3) adresses the same task_struct referencing issue for disconnect
signals
I hope this now finally is the last take ;)
[USBDEVIO] Fix Oops in usbdevio async URB completion (CAN-2005-3055)
If a process issues an URB from userspace and (starts to) terminate
before the URB comes back, we run into the issue described above. This
is because the urb saves a pointer to "current" when it is posted to the
device, but there's no guarantee that this pointer is still valid
afterwards.
This basically means that any userspace process with permissions to
any arbitrary USB device can Ooops any kernel.(!)
In fact, there are two separate issues:
1) the pointer to "current" can become invalid, since the task could be
completely gone when the URB completion comes back from the device.
2) Even if the saved task pointer is still pointing to a valid task_struct,
task_struct->sighand could have gone meanwhile. Therefore, the USB
async URB completion handler needs to reliably check whether
task_struct->sighand is still valid or not. In order to prevent a
race with __exit_sighand(), it needs to grab a
read_lock(&tasklist_lock). This strategy seems to work, since the
send_sig_info() code uses the same protection.
However, we now would need to export a __send_sig_info() function, one
that expects to be called with read_lock(&tasklist_lock) already held by
the caller.
So what we do instead, is to save the PID of the process, and introduce a
new kill_proc_info_as_uid() function. Yes, pid's can and will wrap, so
this is just a short-term fix until usbfs2 will appear.
Signed-off-by: Harald Welte <[email protected]>
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;
+ uid_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);
}
@@ -525,9 +529,11 @@ static int usbdev_open(struct inode *ino
INIT_LIST_HEAD(&ps->async_pending);
INIT_LIST_HEAD(&ps->async_completed);
init_waitqueue_head(&ps->wait);
- ps->discsignr = 0;
- ps->disctask = current;
- ps->disccontext = NULL;
+ ps->disc.signr = 0;
+ ps->disc.pid = current->pid;
+ ps->disc.uid = current->uid;
+ ps->disc.euid = current->euid;
+ ps->disc.context = NULL;
ps->ifclaimed = 0;
wmb();
list_add_tail(&ps->list, &dev->filelist);
@@ -988,7 +994,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);
@@ -1203,8 +1211,8 @@ static int proc_disconnectsignal(struct
return -EFAULT;
if (ds.signr != 0 && (ds.signr < SIGRTMIN || ds.signr > SIGRTMAX))
return -EINVAL;
- ps->discsignr = ds.signr;
- ps->disccontext = ds.context;
+ ps->disc.signr = ds.signr;
+ ps->disc.context = ds.context;
return 0;
}
diff --git a/drivers/usb/core/inode.c b/drivers/usb/core/inode.c
--- a/drivers/usb/core/inode.c
+++ b/drivers/usb/core/inode.c
@@ -708,12 +708,14 @@ void usbfs_remove_device(struct usb_devi
ds = list_entry(dev->filelist.next, struct dev_state, list);
wake_up_all(&ds->wait);
list_del_init(&ds->list);
- if (ds->discsignr) {
+ if (ds->disc.signr) {
sinfo.si_signo = SIGPIPE;
sinfo.si_errno = EPIPE;
sinfo.si_code = SI_ASYNCIO;
- sinfo.si_addr = ds->disccontext;
- send_sig_info(ds->discsignr, &sinfo, ds->disctask);
+ sinfo.si_addr = ds->disc.context;
+ kill_proc_info_as_uid(ds->disc.signr, &sinfo,
+ ds->disc.pid, ds->disc.uid,
+ ds->disc.euid);
}
}
usbfs_update_special();
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -51,9 +51,12 @@ struct dev_state {
struct list_head async_pending;
struct list_head async_completed;
wait_queue_head_t wait; /* wake up if a request completed */
- unsigned int discsignr;
- struct task_struct *disctask;
- void __user *disccontext;
+ struct {
+ unsigned int signr;
+ pid_t pid;
+ uid_t uid, euid;
+ void __user *context;
+ } disc;
unsigned long ifclaimed;
};
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, uid_t, uid_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
@@ -1193,6 +1193,42 @@ kill_proc_info(int sig, struct siginfo *
return error;
}
+/* This is like kill_proc_info(), but doesn't use uid/euid of "current".
+ * Most likely this function is not what you want, it was added as a special
+ * case for usbdevio */
+int
+kill_proc_info_as_uid(int sig, struct siginfo *info, pid_t pid,
+ uid_t uid, uid_t euid)
+{
+ int ret = -EINVAL;
+ struct task_struct *p;
+
+ if (!valid_signal(sig))
+ return ret;
+
+ read_lock(&tasklist_lock);
+ p = find_task_by_pid(pid);
+ if (!p) {
+ ret = -ESRCH;
+ goto out_unlock;
+ }
+ if ((!info || ((unsigned long)info != 1 &&
+ (unsigned long)info != 2 && SI_FROMUSER(info)))
+ && (euid ^ p->suid) && (euid ^ p->uid)
+ && (uid ^ p->suid) && (uid ^ p->uid)) {
+ ret = -EPERM;
+ goto out_unlock;
+ }
+ if (sig && p->sighand) {
+ unsigned long flags;
+ spin_lock_irqsave(&p->sighand->siglock, flags);
+ ret = __group_send_sig_info(sig, info, p);
+ spin_unlock_irqrestore(&p->sighand->siglock, flags);
+ }
+out_unlock:
+ read_unlock(&tasklist_lock);
+ return ret;
+}
/*
* kill_something_info() interprets pid in interesting ways just like kill (2).
@@ -1974,6 +2010,7 @@ EXPORT_SYMBOL(flush_signals);
EXPORT_SYMBOL(force_sig);
EXPORT_SYMBOL(kill_pg);
EXPORT_SYMBOL(kill_proc);
+EXPORT_SYMBOL_GPL(kill_proc_info_as_uid);
EXPORT_SYMBOL(ptrace_notify);
EXPORT_SYMBOL(send_sig);
EXPORT_SYMBOL(send_sig_info);
On Tue, 27 Sep 2005 17:58:00 +0100, Alan Cox <[email protected]> wrote:
> On Maw, 2005-09-27 at 09:09 -0700, Linus Torvalds wrote:
> > > root-owned), then the urb completes, and kill_proc_info() sends the
> > > signal to the unsuspecting process.
> >
> > Ehh.. pid's don't get re-used until they wrap.
>
> Which doesn't take very long to arrange. Relying on pids is definitely a
> security problem we don't want to make worse than it already is.
The whole application cannot exit and leave URBs running behind,
because usbdevio_release() blocks until they are terminated.
Only separate threads can exit.
So, the only thing a malicious user can do is something like this:
- open /proc/bus/usb/BUS/DEV
- submit URB
- fork
- exit parent thread
- wait in the child until PIDs wrap very close to former parent
- exit and hope that someone forks while the exit is processing
Right? But if so, why don't we do something like this:
submit_urb()
as->pid = current->pid;
as->tgid = current->tgid;
.....
async_complete()
__kill_same_process(as->pid, as->tgid);
/* DO NOT USE IN DRIVERS (other than USB core) */
__kill_same_process(pid_t pid, pid_t tgid) {
task_struct *we, *maybe_parent;
lock(&tasklist_lock);
we = find_task_by_pid(pid);
maybe_parent = find_task_by_tgid(pid);
if (maybe_parent != NULL && we->parent == maybe_parent)
send_sig_info(sig, info, we);
unlock(&tasklist_lock);
}
This does not need to check any IDs, I think. Then we do not have to
ponder if effective or real is more appropriate, and if any sort of
new-fanged security thingies like capabilities apply.
-- Pete
On Thu, 13 Oct 2005, Pete Zaitcev wrote:
>
> The whole application cannot exit and leave URBs running behind,
> because usbdevio_release() blocks until they are terminated.
Wrong.
You just do a fork(), and a close in the parent.
"release()" won't be called until the _last_ close, and the task that
opened the fd can certainly exit before that.
If you want to have something that is called on each close, you can
trigger on the "flush()" VFS call, but then you have to realize that that
can happen an infinite number of times, and while the original one is open
(ie on a fork, if the _child_ calls close(), then you'll get a flush, even
though the original fd is still open and still in use).
So you really don't want to terminate the URB's in "flush()". Which means
that the original process can very much be long gone when release happens.
It's a fundamental mistake to think that file descriptors stay with the
process that opened them.
Linus
On Thu, 13 Oct 2005 16:16:58 -0700 (PDT), Linus Torvalds <[email protected]> wrote:
> "release()" won't be called until the _last_ close, and the task that
> opened the fd can certainly exit before that.
>[...]
> It's a fundamental mistake to think that file descriptors stay with the
> process that opened them.
I am quite aware and my proposal takes it into account. I am sorry that
I failed to explain it adequately.
Did you even look at the pseudocode though?
-- Pete
P.S.
submit_urb()
as->pid = current->pid;
as->tgid = current->tgid;
.....
async_complete()
__kill_same_process(as->pid, as->tgid);
/* DO NOT USE IN DRIVERS (other than USB core) */
__kill_same_process(pid_t pid, pid_t tgid) {
task_struct *we, *maybe_parent;
lock(&tasklist_lock);
we = find_task_by_pid(pid);
maybe_parent = find_task_by_tgid(pid);
if (maybe_parent != NULL && we->parent == maybe_parent)
send_sig_info(sig, info, we);
unlock(&tasklist_lock);
}