On Wed, 8 Jul 2009, Oliver Neukum wrote:
> Hi,
>
> I suspect the following race:
>
> CPU A CPU B
>
> spin_lock(&ps->lock);
> list_move_tail(&as->asynclist, &ps->async_completed);
> spin_unlock(&ps->lock);
>
> if (!(as = async_getcompleted(ps)))
> return -EAGAIN;
> return processcompl(as, (void __user * __user *)arg);
>
> processcompl() calls free_async() which calls kfree(as)
>
> as->status = urb->status;
> if (as->signr) {
> sinfo.si_signo = as->signr;
> sinfo.si_errno = as->status;
> sinfo.si_code = SI_ASYNCIO;
> sinfo.si_addr = as->userurb;
> kill_pid_info_as_uid(as->signr, &sinfo, as->pid, as->uid,
> as->euid, as->secid);
> }
> snoop(&urb->dev->dev, "urb complete\n");
> snoop_urb(urb, as->userurb);
>
> Here we are writing into freed memory.
You are right. The spinlock should be held until the end of
async_completed (just before the wake_up). If necessary, the
kill_pid_info_as_uid call should go afterward.
> I am not sure we should do snoop_urb()
> in hard interrupt. What do you think?
There's no reason not to. All it does is a couple of dev_info calls.
Or, that's all it will do once Greg has merged all the patches waiting
in his queue.
For the stable kernels this is more of an issue. However it should
still be okay, considering that people don't use usbfs_snoop very much.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
