On Wed, 4 Feb 2015, Al Viro wrote:
> [USB folks Cc'd]
Incidentally, Al, have you seen this email?
http://marc.info/?l=linux-usb&m=142295011402339&w=2
I encouraged the writer to send in a patch but so far there has been no
reply.
> On Mon, Feb 02, 2015 at 03:26:17PM +0100, Christoph Hellwig wrote:
>
> > I would bet the behavior difference is a bug, might be worth to Cc the
> > usb folks on this issue. I bet we'd want the more complex behavior
> > for both variants.
>
> [Context for USB people: The difference in question is what ep_read() does
> when it is called on write endpoint that isn't isochronous;
You're talking about drivers/usb/gadget/legacy/inode.c, right?
> it halts the
> sucker and fails with EBADMSG, while ep_aio_read() handles all write endpoints
> as isochronous ones - fails with EINVAL; FWIW, I agree that it's probably
> a bug]
It's not a bug; it's by design. That's how you halt an endpoint in
gadgetfs -- by doing a synchronous I/O call in the "wrong" direction.
> Sadly, that's not the only problem in there ;-/ _This_ one really has
> the "what if single-segment AIO read tries to dereference iovec after
> the caller is gone" bug you suspected in fs/direct-io.c; we have
> static void ep_user_copy_worker(struct work_struct *work)
> {
> struct kiocb_priv *priv = container_of(work, struct kiocb_priv, work);
> struct mm_struct *mm = priv->mm;
> struct kiocb *iocb = priv->iocb;
> size_t ret;
>
> use_mm(mm);
> ret = ep_copy_to_user(priv);
> unuse_mm(mm);
>
> /* completing the iocb can drop the ctx and mm, don't touch mm after
> */
> aio_complete(iocb, ret, ret);
>
> kfree(priv->buf);
> kfree(priv);
> }
> called via schedule_work() from ->complete() of usb_request allocated and
> queued by ->aio_read(). It very definitely _can_ be executed after return
> from ->aio_read() and aio_run_iocb(). And ep_copy_to_user() dereferences
> the iovec given to ->aio_read(); _not_ its copy as f_fs.c does.
>
> Do io_submit(2) with several IOCB_CMD_PREAD requests, and you'll almost
> certainly get the data from the first one copied to the destination of
> the second one instead. It shouldn't be hard to reproduce. And that,
> of course, is not the worst possible outcome...
>
> I'm going to add copying of iovec in async read case. And AFAICS, that one
> is -stable fodder. See vfs.git#gadget for f_fs.c conversion; I haven't
> pushed legacy/inode.c stuff yet - I need to pull the fix of the bug above
> into the beginning of that pile first.
>
> FWIW, I don't believe that it makes sense to do iovec copying in
> aio_run_iocb(); note that most of the instances will be done with
> iovec before they return there.
That's true even for gadgetfs in the write case.
> These two were the sole exceptions;
> function/f_fs.c did copying, legacy/inode.c didn't. Most of the
> ->aio_read/->read_iter instances (including ones that *do* return
> EIOCBQUEUED) only access iovec synchronously; usually that's done
> by grabbing the pages to copy into before we get aronud to starting
> IO. legacy/inode.c is the only instance to step into that kind of bug.
> function/f_fs.c also had a fun bug, BTW - failure in AIO ended up leaking
> io_data (plus iovec copy in case of aio_read()). Looks like another
> -stable fodder, if less serious one... See b17d2ded6 (gadget/function/f_fs.c:
> close leaks) in vfs.git#gadget for that one.
>
> I plan to pull the fix for use-after-free in the beginning of that queue
> (in an easy to backport form) and then have ep_aio_read/ep_aio_write
> start doing the halt-related bits as in ep_read/ep_write. With that it's
> trivial to convert that sucker along the same lines as function/f_fs.c.
I don't think there's any need to make the async routines do the
halt-related stuff. After all, it's silly for users to call an async
I/O routine to perform a synchronous action like halting an endpoint.
On the other hand, it would be reasonable to replace the -EBADMSG with
some massaged version of the return code from usb_ep_set_halt(), which
is supposed to return -EAGAIN under some circumstances. But that would
be an API change, so we probably shouldn't do it...
> All of that, assuming that anybody gives a damn about the driver in question.
> The things like
> spin_lock_irq (&dev->lock);
> ....
> // FIXME don't call this with the spinlock held ...
> if (copy_to_user (buf, dev->req->buf, len))
> seem to indicate that nobody does, seeing that this bug had been there
> since 2003, complete with FIXME ;-/
>
> If nobody cares about that sucker, git rm would be a better solution, IMO...
It is a legacy driver after all, but some people still use it.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
