Re: BUG: bad usercopy in hidraw_ioctl
On Wed, Aug 07, 2019 at 12:58:21PM -0700, Matthew Wilcox wrote:
> On Wed, Aug 07, 2019 at 12:28:06PM -0700, syzbot wrote:
> > usercopy: Kernel memory exposure attempt detected from wrapped address
> > (offset 0, size 0)!
> > [ cut here ]
> > kernel BUG at mm/usercopy.c:98!
>
> This report is confusing because the arguments to usercopy_abort() are wrong.
>
> /* Reject if object wraps past end of memory. */
> if (ptr + n < ptr)
> usercopy_abort("wrapped address", NULL, to_user, 0, ptr + n);
>
> ptr + n is not 'size', it's what wrapped. I don't know what 'offset'
> should be set to, but 'size' should be 'n'. Presumably we don't want to
> report 'ptr' because it'll leak a kernel address ... reporting 'n' will
> leak a range for a kernel address, but I think that's OK? Admittedly an
> attacker can pass in various values for 'n', but it'll be quite noisy
> and leave a trace in the kernel logs for forensics to find afterwards.
>
> > Call Trace:
> > check_bogus_address mm/usercopy.c:151 [inline]
> > __check_object_size mm/usercopy.c:260 [inline]
> > __check_object_size.cold+0xb2/0xba mm/usercopy.c:250
> > check_object_size include/linux/thread_info.h:119 [inline]
> > check_copy_size include/linux/thread_info.h:150 [inline]
> > copy_to_user include/linux/uaccess.h:151 [inline]
> > hidraw_ioctl+0x38c/0xae0 drivers/hid/hidraw.c:392
>
> The root problem would appear to be:
>
> else if (copy_to_user(user_arg + offsetof(
> struct hidraw_report_descriptor,
> value[0]),
> dev->hid->rdesc,
> min(dev->hid->rsize, len)))
>
> That 'min' should surely be a 'max'?
Surely not. ->rsize is the amount of data available to copy out; len
is the size of buffer supplied by userland to copy into.
BTW, why is it playing those games with offsetof, anyway? What's wrong
with
struct hidraw_report_descriptor __user *p = user_arg;
...
get_user(&p->size)
...
copy_to_user(p->value, ...)
Re: [PATCH v2 05/17] compat_ioctl: move more drivers to generic_compat_ioctl_ptrarg
On Fri, Sep 14, 2018 at 01:35:06PM -0700, Darren Hart wrote: > Acked-by: Darren Hart (VMware) > > As for a longer term solution, would it be possible to init fops in such > a way that the compat_ioctl call defaults to generic_compat_ioctl_ptrarg > so we don't have to duplicate this boilerplate for every ioctl fops > structure? Bad idea, that... Because several years down the road somebody will add an ioctl that takes an unsigned int for argument. Without so much as looking at your magical mystery macro being used to initialize file_operations. FWIW, I would name that helper in more blunt way - something like compat_ioctl_only_compat_pointer_ioctls_here()...
Re: [GIT PULL] USB/PHY driver patches for 4.17-rc1
On Thu, Apr 05, 2018 at 12:31:11AM -0700, Christoph Hellwig wrote: > On Thu, Apr 05, 2018 at 09:19:28AM +0200, Lars-Peter Clausen wrote: > > On 04/05/2018 08:31 AM, Kees Cook wrote: > > > On Wed, Apr 4, 2018 at 3:31 AM, Greg KH > > > wrote: > > >> Lars-Peter Clausen (2): > > >> usb: gadget: ffs: Execute copy_to_user() with USER_DS set > > > > > > https://git.kernel.org/linus/4058ebf33cb0be88ca516f968eda24ab7b6b93e4 > > > > > > Isn't there a better way to do this without the set_fs() usage? We've > > > been try to eliminate it in the kernel. I thought there was a safer > > > way to use iters now? > > > > The problem is use_mm(). It needs to be accompanied with set_fs(DS_USER) to > > work reliably. This has simply been missing for this particular instance of > > use_mm(). > > To me it seems like use_mm() should do set_fs(USER_DS) and unuse_mm() > should do set_fs(KERNEL_DS) to get drivers outo of this mess. I'll see > if I can come up with patches for the next merge window. Yes on the former, not quite on the latter (we need to go back to the state before use_mm()). Said that, drm users of use_mm() look rather fishy and might be worth a good look... -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: f_fs: use memdup_user
On Sat, May 13, 2017 at 11:05:30AM +0300, Dan Carpenter wrote:
> > + data = memdup_user(buf, len);
> > + if (unlikely(IS_ERR(data)))
>
> Don't use likely/unlikely() here. It's not a fast path.
More to the point,
#define IS_ERR_VALUE(x) unlikely((unsigned long)(void *)(x) >= (unsigned
long)-MAX_ERRNO)
static inline bool __must_check IS_ERR(__force const void *ptr)
{
return IS_ERR_VALUE((unsigned long)ptr);
}
IOW, IS_ERR() already produces unlikely(), fast path or not.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: f_fs: Fix incorrect EFAULT generation for async read operations
On Mon, Mar 28, 2016 at 02:42:43PM +0200, Lars-Peter Clausen wrote:
> In the current implementation functionfs generates a EFAULT for async read
> operations if the read buffer size is larger than the URB data size. Since
> a application does not necessarily know how much data the host side is
> going to send it typically supplies a buffer larger than the actual data,
> which will then result in a EFAULT error.
>
> This behaviour was introduced while refactoring the code to use iov_iter
> interface in commit c993c39b8639 ("gadget/function/f_fs.c: use put iov_iter
> into io_data"). The original code took the minimum over the URB size and
> the user buffer size and then attempt to copy that many bytes using
> copy_to_user(). If copy_to_user() returned less copied bytes the code would
> generate a EFAULT error. This has exactly the same behaviour as using
> copy_to_iter() except that copy_to_iter() can't fail since the check
> whether access to the user supplied buffer is OK is already done earlier
> and copy_to_iter() wont be called in that case. Restore the original
> behaviour by simply dropping the code that generates the EFAULT error.
"The check whether access to user supplied buffer is OK" is *not* done
before. access_ok() checked does not guarantee that copy_to_user() won't
fail.
access_ok() and its equivalents checks only one thing - that you are not
trying to pass kernel addresses for userland ones. It does not check
permissions you have for individual pages, it does not check if they are
present, or, if absent, that page fault will bring them in.
Both copy_to_user() and copy_to_iter() can bloody well fail halfway through
without access_ok() having any objections. E.g. do read() into an mmapped
buffer, with one page somewhere in the middle munmapped. Or the first page
munmapped, for that matter...
I see where the commit in question is buggy, but your patch doesn't really
fix it; original behaviour is not restored.
- ret = min_t(size_t, ret, io_data->len);
-
- if (unlikely(copy_to_user(io_data->buf,
- data, ret)))
+ ret = copy_to_iter(data, ret,
&io_data->data);
+ if
(unlikely(iov_iter_count(&io_data->data)))
ret = -EFAULT;
was wrong; what it should've done to preserve the original behaviour is
something like
copied = copy_to_iter(data, ret, &io_data->data)
if (unlikely(copied != ret))
ret = iov_iter_count(&io_data->data)
? EFAULT : copied;
After copy_to_iter(data, len, target) we could have three outcomes:
* everything copied; return value is equal to len
* copying stopped at the end of iterator; return value is less than len,
iov_iter_count(target) is zero.
* copying stopped at unmapped page/page that was readonly/etc;
return value is less than len, iov_iter_count(target) is *not* zero.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] USB: inode.c: fix unbalanced spin_lock in ep0_write
On Fri, Dec 11, 2015 at 08:56:26PM +0100, David Eccher wrote: > Fix bad unlock balance: ep0_write enter with the locks locked from > inode.c:1769, > hence it must exit with spinlock held to avoid double unlock in dev_config. *Ugh* Just take that spinlock before the if (retval < 0) and don't drop it after clear_req(), then... -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Locking issues w/ functionfs gadget and aio?
On Fri, Jun 12, 2015 at 05:51:12PM -0700, John Stultz wrote:
> I'm not super sure what the right fix is, but if do something like the
> following (sorry, whitespace corrupted via copy/paste), I don't seem
> to run into the problem.
Looks sane. Which tree would you prefer it to go through, vfs or usb?
BTW, in either case you'd need Signed-off-by: on that patch...
> diff --git a/drivers/usb/gadget/function/f_fs.c
> b/drivers/usb/gadget/function/f_fs.c
> index 3507f88..e322818 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -924,7 +924,8 @@ static ssize_t ffs_epfile_write_iter(struct kiocb
> *kiocb, struct iov_iter *from)
>
> kiocb->private = p;
>
> - kiocb_set_cancel_fn(kiocb, ffs_aio_cancel);
> + if (p->aio)
> + kiocb_set_cancel_fn(kiocb, ffs_aio_cancel);
>
> res = ffs_epfile_io(kiocb->ki_filp, p);
> if (res == -EIOCBQUEUED)
> @@ -968,7 +969,8 @@ static ssize_t ffs_epfile_read_iter(struct kiocb
> *kiocb, struct iov_iter *to)
>
> kiocb->private = p;
>
> - kiocb_set_cancel_fn(kiocb, ffs_aio_cancel);
> + if(p->aio)
> + kiocb_set_cancel_fn(kiocb, ffs_aio_cancel);
>
> res = ffs_epfile_io(kiocb->ki_filp, p);
> if (res == -EIOCBQUEUED)
>
>
> Is there a better solution here? I'm not sure I see if the
> is_sync_kiocb(kiocb) check would ever be false from here, so I'm not
> sure if all the p->aio checking is really needed or not.
>
> This issue seems to have been introduced with 70e60d917e91fff
> (gadget/function/f_fs.c: switch to ->{read,write}_iter())
>
> thanks
> -john
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
Re: [PATCH] usb: gadget: f_fs: do not set cancel function on synchronous {read,write}
On Wed, May 27, 2015 at 11:41:31AM +0100, Rui Miguel Silva wrote:
> do not try to set cancel function in synchronous operations in
> ffs_epfile_{read,write}_iter.
>
> With, 70e60d917 gadget/function/f_fs.c: switch to ->{read,write}_iter()
> if CONFIG_AIO is disable there is no problem as kiocb_set_cancel_fn
> is a nop, with this option enabled it will try to use ctx that is not
> allocated for synchronous operations. And for that will dereference a
> null at the set cancel function in any synchronous read/write.
ACK. And it's -stable fodder for 4.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git pull] gadgetfs fixes
On Sun, Mar 15, 2015 at 09:50:01AM +0100, Alexander Holler wrote: > Hmm, a year ago or so I've stumbled over the fact that the owner > might be necessary for correct entries in sysfs (that was mtd). I've > no idea if that's true here too but it might be worse to mention it. There are two mechanisms. One keeps the filesystem your file is on pinned while the file is opened. That works for all filesystems, and it's enough when the methods of that file are in the module defining that filesystem. In cases when that is not enough (e.g. character device living on ext3 - it's nice to have ext3 pinned down, but you want the driver that defined that device to be pinned as well) you can ask to pin a given module for as long as the file is opened. That's what file_operations ->owner gives. Your example (mtd creating a file on sysfs) also needs that - you want mtd pinned, not just sysfs. gadgetfs, however, has filesystem defined by the same module. So pinning filesystem_type down is enough - nothing extra is needed, same as for e.g. a regular file on ext3. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git pull] gadgetfs fixes
On Sun, Mar 15, 2015 at 07:35:20AM +0100, Alexander Holler wrote: > > Umm... If I'm not misparsing what you said, you are talking about the > > Glücklicherweise nicht. Vielleicht sollten wir es zur Abwechslung mal > mit meiner bevorzugten Sprache versuchen. Good. I'll probably abstain from trying to mangle it, though. Another question, if you don't mind - does that series (i.e. what's currently in Linus' tree) fix the module refcount issues you'd been seeing? I agree with your analysis of likely cause (->f_op reassignments with different ->owner before and after) and these patches should have eliminated that, but confirmation would be nice... Incidentally, none of those file_operations need ->owner in the first place; it doesn't hurt (as long as ->f_op doesn't change that way), but such files (living on a filesystem provided by the module their methods are in) don't need the module refcount bumped while the file is opened - having it opened pins file_system_type of containing filesystem (by keeping a reference to struct vfsmount, which keeps a reference to struct super_block, which keeps a reference to file_system_type), so the module will be kept busy just fine. Again, having ->owner on file_operations doesn't hurt, so it's not a bug per se - just pointless in such cases. So we might want to remove it from ep_io_operations someday. Anyway, that's a completely separate story... -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git pull] gadgetfs fixes
On Sun, Mar 15, 2015 at 01:39:21AM +0100, Alexander Holler wrote:
> Am 13.03.2015 um 17:42 schrieb Al Viro:
> > Assorted fixes around AIO on gadgetfs: leaks, use-after-free,
> > troubles caused by ->f_op flipping. Please, pull from
> > git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git gadget
> >
> > Shortlog:
> > Al Viro (8):
> > new helper: dup_iter()
> > move iov_iter.c from mm/ to lib/
> > gadget/function/f_fs.c: close leaks
> > gadget/function/f_fs.c: use put iov_iter into io_data
> > gadget/function/f_fs.c: switch to ->{read,write}_iter()
>
> > gadgetfs: use-after-free in ->aio_read()
>
> If that patch ends up in the stable kernels (as it is marked as such),
> it needs a
> value = -ENOMEM
> before that added "goto fail;", otherwise the return value is unitialized.
Umm... If I'm not misparsing what you said, you are talking about the
one that gets removed by
- if (iv) {
- priv->iv = kmemdup(iv, nr_segs * sizeof(struct iovec),
- GFP_KERNEL);
- if (!priv->iv) {
- kfree(priv);
- goto fail;
- }
- }
in "gadget: switch ep_io_operations to ->read_iter/->write_iter" very
shortly afterwards, and _that_ is a prereq for ->f_op flipping fixes,
which is also clear -stable fodder. But yes, it's a bisect hazard and
a cherry-pick one as well. Nice catch...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[git pull] gadgetfs fixes
Assorted fixes around AIO on gadgetfs: leaks, use-after-free,
troubles caused by ->f_op flipping. Please, pull from
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git gadget
Shortlog:
Al Viro (8):
new helper: dup_iter()
move iov_iter.c from mm/ to lib/
gadget/function/f_fs.c: close leaks
gadget/function/f_fs.c: use put iov_iter into io_data
gadget/function/f_fs.c: switch to ->{read,write}_iter()
gadgetfs: use-after-free in ->aio_read()
gadget: switch ep_io_operations to ->read_iter/->write_iter
gadgetfs: get rid of flipping ->f_op in ep_config()
Alan Stern (1):
gadgetfs: really get rid of switching ->f_op
Diffstat:
drivers/usb/gadget/function/f_fs.c | 204 +++-
drivers/usb/gadget/legacy/inode.c | 466 +++--
include/linux/uio.h| 2 +
lib/Makefile | 2 +-
{mm => lib}/iov_iter.c | 15 ++
mm/Makefile| 2 +-
6 files changed, 291 insertions(+), 400 deletions(-)
rename {mm => lib}/iov_iter.c (97%)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: linux-next: manual merge of the net-next tree with the vfs tree
On Fri, Mar 13, 2015 at 03:38:17PM +1100, Stephen Rothwell wrote:
> There is also a conflict with e9eab93cc2dc ("fs: don't allow to
> complete sync iocbs through aio_complete"), though it doesn't show up
> in the resolution since I I just used the next-next tree bits. So a
> common branch containing that as well could be merged into both trees.
OK, for now I've done just that (vfs.git#iocb in never-rebase mode).
I still think that vfs.git#gadget ought to go into mainline; arguments
for the rest of #iocb are weaker and merging it into net-next would
suffice; as the matter of fact, I have pending stuff for net-next touching
the same area (further reduction of ->sendmsg()/->recvmsg() argument lists;
total_len is redundant); might as well deal with that when feeding that
to Dave...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: linux-next: manual merge of the net-next tree with the vfs tree
On Thu, Mar 12, 2015 at 11:24:26PM -0400, David Miller wrote:
> From: Stephen Rothwell
> Date: Fri, 13 Mar 2015 13:15:43 +1100
>
> > Today's linux-next merge of the net-next tree got a conflict in
> > net/socket.c between commits 005139a14660 ("fs: remove ki_nbytes") and
> > e9eab93cc2dc ("fs: don't allow to complete sync iocbs through
> > aio_complete") from the vfs tree and commit 1b784140474e ("net: Remove
> > iocb argument from sendmsg and recvmsg") from the net-next tree.
> >
> > I fixed it up (mainly using the net-next version - see below) and can
> > carry the fix as necessary (no action is required).
>
> Al, how do you want to resolve this?
Hmm... I could backmerge 1b784140474e4fc94281a49e96c67d29df0efbde into
vfs.git#for-next, of course, but you've got quite a pile of stuff in front
of it... FWIW, the conflict resolution proposed by Stephen is correct;
the question is what should go into which tree.
Actually, prereqs of the commit in question on vfs.git side are mostly
-stable fodder; all it really needs is vfs.git#gadget and I was planning
to send that to Linus - fixes for leaks and use-after-free in gadgetfs
that had been there since forever, plus fixes for regression since 3.18
(->f_op flipping that had always been fishy and outright broke when we
started to FMODE_CAN_READ/FMODE_CAN_WRITE). USB folks seem to be OK
with it. Christoph's patch isn't a regression fix, but seeing that it's
(a) trivial and (b) ends up causing merge headache... Maybe it would
make sense to pull it into mainline and resolve the conflict on backmerge
from mainline to net-next. Linus? I've pushed that (gadget + ki_nbytes)
into vfs.git#for-linus-2; would you be OK with pulling that?
It's on
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-linus-2
Shortlog:
Al Viro (8):
new helper: dup_iter()
move iov_iter.c from mm/ to lib/
gadget/function/f_fs.c: close leaks
gadget/function/f_fs.c: use put iov_iter into io_data
gadget/function/f_fs.c: switch to ->{read,write}_iter()
gadgetfs: use-after-free in ->aio_read()
gadget: switch ep_io_operations to ->read_iter/->write_iter
gadgetfs: get rid of flipping ->f_op in ep_config()
Alan Stern (1):
gadgetfs: really get rid of switching ->f_op
Christoph Hellwig (1):
fs: remove ki_nbytes
Diffstat:
drivers/usb/gadget/function/f_fs.c | 204 +++-
drivers/usb/gadget/legacy/inode.c | 466 +++--
fs/aio.c | 34 +--
fs/ceph/file.c | 2 +-
fs/nfs/direct.c| 2 +-
fs/ocfs2/file.c| 8 +-
fs/read_write.c| 8 -
fs/udf/file.c | 2 +-
include/linux/aio.h| 1 -
include/linux/uio.h| 2 +
kernel/printk/printk.c | 2 +-
lib/Makefile | 2 +-
{mm => lib}/iov_iter.c | 15 ++
mm/Makefile| 2 +-
mm/page_io.c | 1 -
net/socket.c | 6 +-
16 files changed, 319 insertions(+), 438 deletions(-)
rename {mm => lib}/iov_iter.c (97%)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
On Sun, Mar 08, 2015 at 02:35:25PM -0400, Alan Stern wrote: > > FWIW, I've pushed those two fixes in vfs.git#gadget; could I have your > > s-o-b on the ep0 part? See 2b13438 in vfs.git... > > Certainly. > > Signed-off-by: Alan Stern Amended and pushed... -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
On Sat, Mar 07, 2015 at 04:08:49PM -0500, Alan Stern wrote: > On Sat, 7 Mar 2015, Alexander Holler wrote: > > > Am 07.03.2015 um 12:23 schrieb Alexander Holler: > > > Am 04.03.2015 um 16:31 schrieb Alan Stern: > > > > > >> check to see what those values actually were. It's easy enough to fix > > >> up, though; revised patch below. > > > > > > Thanks, in contrast to the patch from Al Viro that one applies. > > > > And as I've just tested it, it isn't complete. ep_config_operations will > > be switched to ep_io_operations and suffers under the same problem of > > not having initially (aio_)read/(aio_)write since commit 7f7f25e8 (3.16). > > Of course. I stated in the email accompanying the original version of > this patch that it contained some corrections that Al's patch left out. > You have to use the two of them together to fix all the issues. FWIW, I've pushed those two fixes in vfs.git#gadget; could I have your s-o-b on the ep0 part? See 2b13438 in vfs.git... -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
On Sat, Mar 07, 2015 at 09:03:51PM +0100, Alexander Holler wrote: > Am 07.03.2015 um 12:23 schrieb Alexander Holler: > > Am 04.03.2015 um 16:31 schrieb Alan Stern: > > > >> check to see what those values actually were. It's easy enough to fix > >> up, though; revised patch below. > > > > Thanks, in contrast to the patch from Al Viro that one applies. Translation: it applies to mainline as well as to vfs.git#gadget + ep_config_operations patch it's incremental to. > And as I've just tested it, it isn't complete. ep_config_operations will > be switched to ep_io_operations and suffers under the same problem of > not having initially (aio_)read/(aio_)write since commit 7f7f25e8 (3.16). Incremental patch does not deal with the problem handled by the patch it is incremental to, film at 11... FWIW, patches in vfs.git#gadget are fixing fairly obvious bugs - leaks and use-after-free. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
On Tue, Mar 03, 2015 at 10:47:14AM -0500, Alan Stern wrote: > @@ -1279,6 +1284,9 @@ ep0_poll (struct file *fd, poll_table *w > struct dev_data *dev = fd->private_data; > int mask = 0; > > + if (dev->state <= STATE_DEV_OPENED) > + return -ENODEV; > + > poll_wait(fd, &dev->wait, wait); > > spin_lock_irq (&dev->lock); That, BTW, is wrong. ->poll() does *not* return negatives - to imitate "we don't have ->poll()" we need it to return DEFAULT_POLLMASK. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
On Tue, Mar 03, 2015 at 10:47:14AM -0500, Alan Stern wrote:
> On Tue, 3 Mar 2015, Al Viro wrote:
>
> > Looking at that thing again... why do they need to be dummy? After all,
> > those methods start with get_ready_ep(), which will fail unless we have
> > ->state == STATE_EP_ENABLED. So they'd be failing just fine until that
> > first write() anyway. Let's do the following:
>
> In addition to the changes you made, it looks like you will need the
> following or something similar (also untested). I'm not sure if this
> is race-free, but it's better than before.
Right, ep0 has the same kind of problem...
> @@ -1240,6 +1241,10 @@ static int
> ep0_fasync (int f, struct file *fd, int on)
> {
> struct dev_data *dev = fd->private_data;
> +
> + if (dev->state <= STATE_DEV_OPENED)
> + return -ENODEV;
> +
Er... What is protecting dev->state here? Matter of fact, what's the
point of that check at all? Right now you have .fasync = ep0_fasync
both in ep0_io_operations and in dev_init_operations, so your delta
changes the existing semantics...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
On Mon, Mar 02, 2015 at 02:02:56PM +0100, Alexander Holler wrote:
> >I exactly did what you've assumed, I've just fixed f_mode but not the
> >already existing races which I haven't introduced. So I was right in not
> >sending a patch as would have been blamed for not rewriting everything
> >as so often.
>
> Another quick solution would be to just add some dummy ops for
> read/write to those file_operations which are missing it which are
> only returning -EINVAL or some other error which might make sense.
Looking at that thing again... why do they need to be dummy? After all,
those methods start with get_ready_ep(), which will fail unless we have
->state == STATE_EP_ENABLED. So they'd be failing just fine until that
first write() anyway. Let's do the following:
* get_ready_ep() gets a new argument - true when called from
ep_write_iter(), false otherwise.
* make it quiet when it finds STATE_EP_READY (no printk, that is;
the case won't be impossible after that change).
* when that new argument is true, treat STATE_EP_READY the same
way as STATE_EP_ENABLED (i.e. return zero and do not unlock).
* in ep_write_iter(), after success of get_ready_ep() turn
if (!usb_endpoint_dir_in(&epdata->desc)) {
into
if (epdata->state == STATE_EP_ENABLED &&
!usb_endpoint_dir_in(&epdata->desc)) {
- that logics only applies after config.
* have ep_config() take kernel-side buffer (i.e. use memcpy()
instead of copy_from_user() in there) and in the "let's call ep_io or
ep_aio" (again, in ep_write_iter()) add "... or ep_config() in case it's
not configured yet"
IOW, how about the following (completely untested) patch on top of
vfs.git#gadget? Comments?
diff --git a/drivers/usb/gadget/legacy/inode.c
b/drivers/usb/gadget/legacy/inode.c
index b825edc..c0e2532 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -74,6 +74,8 @@ MODULE_DESCRIPTION (DRIVER_DESC);
MODULE_AUTHOR ("David Brownell");
MODULE_LICENSE ("GPL");
+static int ep_open(struct inode *, struct file *);
+
/*--*/
@@ -283,14 +285,15 @@ static void epio_complete (struct usb_ep *ep, struct
usb_request *req)
* still need dev->lock to use epdata->ep.
*/
static int
-get_ready_ep (unsigned f_flags, struct ep_data *epdata)
+get_ready_ep (unsigned f_flags, struct ep_data *epdata, bool is_write)
{
int val;
if (f_flags & O_NONBLOCK) {
if (!mutex_trylock(&epdata->lock))
goto nonblock;
- if (epdata->state != STATE_EP_ENABLED) {
+ if (epdata->state != STATE_EP_ENABLED &&
+ (!is_write || epdata->state != STATE_EP_READY)) {
mutex_unlock(&epdata->lock);
nonblock:
val = -EAGAIN;
@@ -305,18 +308,20 @@ nonblock:
switch (epdata->state) {
case STATE_EP_ENABLED:
+ return 0;
+ case STATE_EP_READY:/* not configured yet */
+ if (is_write)
+ return 0;
+ // FALLTHRU
+ case STATE_EP_UNBOUND: /* clean disconnect */
break;
// case STATE_EP_DISABLED: /* "can't happen" */
- // case STATE_EP_READY: /* "can't happen" */
default:/* error! */
pr_debug ("%s: ep %p not available, state %d\n",
shortname, epdata, epdata->state);
- // FALLTHROUGH
- case STATE_EP_UNBOUND: /* clean disconnect */
- val = -ENODEV;
- mutex_unlock(&epdata->lock);
}
- return val;
+ mutex_unlock(&epdata->lock);
+ return -ENODEV;
}
static ssize_t
@@ -390,7 +395,7 @@ static long ep_ioctl(struct file *fd, unsigned code,
unsigned long value)
struct ep_data *data = fd->private_data;
int status;
- if ((status = get_ready_ep (fd->f_flags, data)) < 0)
+ if ((status = get_ready_ep (fd->f_flags, data, false)) < 0)
return status;
spin_lock_irq (&data->dev->lock);
@@ -572,7 +577,7 @@ ep_read_iter(struct kiocb *iocb, struct iov_iter *to)
ssize_t value;
char *buf;
- if ((value = get_ready_ep(file->f_flags, epdata)) < 0)
+ if ((value = get_ready_ep(file->f_flags, epdata, false)) < 0)
return value;
/* halt any endpoint by doing a "wrong direction" i/o call */
@@ -620,20 +625,25 @@ fail:
return value;
}
+static ssize_t ep_config(struct ep_data *, const char *, size_t);
+
static ssize_t
ep_write_iter(struct kiocb *iocb, struct iov_iter *from)
{
struct file *file = iocb->ki_filp;
struct ep_data *epdata = file->private_data;
size_t len = iov_iter_count(from);
+
Re: gadgetfs broken since 7f7f25e8
On Mon, Mar 02, 2015 at 10:13:27AM +0100, Richard Weinberger wrote: > On Mon, Mar 2, 2015 at 9:28 AM, Alexander Holler wrote: > > Hello. > > > > Commit 7f7f25e82d54870df24d415a7007fbd327da027b (introduced with 3.16) broke > > dynamic changing of file_operations->[read|write]. > > > > At least gadgetfs is a victim. > > > > Feel free to ask me off-list for a patch as I don't want to end up in > > annoying discussions on Linux kernel lists anymore. > > > > Alexander Holler > > CC'ing Al. I know. FWIW, gadgetfs is one of the very few places that tried to pull that crap off and it had always been seriously racy. I've posted a partial analysis about a month ago (<20150204190645.gj29...@zeniv.linux.org.uk>). If Alexander (or anybody else) has a patch that really fixes that thing, I would certainly like to see it. If not, I'll try to cook something, but I'm not very familiar with that code. I really hope that this patch isn't "modify ->f_mode to match ->f_op change" - that's too racy. We'll obviously need to fix the userland-visible breakage in that one, but that's not the way to go... -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/6] gadget: switch ep_io_operations to ->read_iter/->write_iter
From: Al Viro
Signed-off-by: Al Viro
---
drivers/usb/gadget/legacy/inode.c | 355 +++---
1 file changed, 141 insertions(+), 214 deletions(-)
diff --git a/drivers/usb/gadget/legacy/inode.c
b/drivers/usb/gadget/legacy/inode.c
index 9fbbaa0..b825edc 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -363,97 +363,6 @@ ep_io (struct ep_data *epdata, void *buf, unsigned len)
return value;
}
-
-/* handle a synchronous OUT bulk/intr/iso transfer */
-static ssize_t
-ep_read (struct file *fd, char __user *buf, size_t len, loff_t *ptr)
-{
- struct ep_data *data = fd->private_data;
- void*kbuf;
- ssize_t value;
-
- if ((value = get_ready_ep (fd->f_flags, data)) < 0)
- return value;
-
- /* halt any endpoint by doing a "wrong direction" i/o call */
- if (usb_endpoint_dir_in(&data->desc)) {
- if (usb_endpoint_xfer_isoc(&data->desc)) {
- mutex_unlock(&data->lock);
- return -EINVAL;
- }
- DBG (data->dev, "%s halt\n", data->name);
- spin_lock_irq (&data->dev->lock);
- if (likely (data->ep != NULL))
- usb_ep_set_halt (data->ep);
- spin_unlock_irq (&data->dev->lock);
- mutex_unlock(&data->lock);
- return -EBADMSG;
- }
-
- /* FIXME readahead for O_NONBLOCK and poll(); careful with ZLPs */
-
- value = -ENOMEM;
- kbuf = kmalloc (len, GFP_KERNEL);
- if (unlikely (!kbuf))
- goto free1;
-
- value = ep_io (data, kbuf, len);
- VDEBUG (data->dev, "%s read %zu OUT, status %d\n",
- data->name, len, (int) value);
- if (value >= 0 && copy_to_user (buf, kbuf, value))
- value = -EFAULT;
-
-free1:
- mutex_unlock(&data->lock);
- kfree (kbuf);
- return value;
-}
-
-/* handle a synchronous IN bulk/intr/iso transfer */
-static ssize_t
-ep_write (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
-{
- struct ep_data *data = fd->private_data;
- void*kbuf;
- ssize_t value;
-
- if ((value = get_ready_ep (fd->f_flags, data)) < 0)
- return value;
-
- /* halt any endpoint by doing a "wrong direction" i/o call */
- if (!usb_endpoint_dir_in(&data->desc)) {
- if (usb_endpoint_xfer_isoc(&data->desc)) {
- mutex_unlock(&data->lock);
- return -EINVAL;
- }
- DBG (data->dev, "%s halt\n", data->name);
- spin_lock_irq (&data->dev->lock);
- if (likely (data->ep != NULL))
- usb_ep_set_halt (data->ep);
- spin_unlock_irq (&data->dev->lock);
- mutex_unlock(&data->lock);
- return -EBADMSG;
- }
-
- /* FIXME writebehind for O_NONBLOCK and poll(), qlen = 1 */
-
- value = -ENOMEM;
- kbuf = memdup_user(buf, len);
- if (IS_ERR(kbuf)) {
- value = PTR_ERR(kbuf);
- kbuf = NULL;
- goto free1;
- }
-
- value = ep_io (data, kbuf, len);
- VDEBUG (data->dev, "%s write %zu IN, status %d\n",
- data->name, len, (int) value);
-free1:
- mutex_unlock(&data->lock);
- kfree (kbuf);
- return value;
-}
-
static int
ep_release (struct inode *inode, struct file *fd)
{
@@ -517,8 +426,8 @@ struct kiocb_priv {
struct mm_struct*mm;
struct work_struct work;
void*buf;
- const struct iovec *iv;
- unsigned long nr_segs;
+ struct iov_iter to;
+ const void *to_free;
unsignedactual;
};
@@ -541,34 +450,6 @@ static int ep_aio_cancel(struct kiocb *iocb)
return value;
}
-static ssize_t ep_copy_to_user(struct kiocb_priv *priv)
-{
- ssize_t len, total;
- void*to_copy;
- int i;
-
- /* copy stuff into user buffers */
- total = priv->actual;
- len = 0;
- to_copy = priv->buf;
- for (i=0; i < priv->nr_segs; i++) {
- ssize_t this = min((ssize_t)(priv->iv[i].iov_len), total);
-
- if (copy_to_user(priv->iv[i].iov_base, to_copy, this)) {
- if (len == 0)
- len = -EFAULT;
- break;
- }
-
- total -= this;
- len
[PATCH 2/6] gadget/function/f_fs.c: close leaks
From: Al Viro
If ffs_epfile_io() fails in AIO case, we end up leaking io_data
(and iovec_copy in case of AIO read).
Signed-off-by: Al Viro
---
drivers/usb/gadget/function/f_fs.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index 63314ed..0c120ad 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -962,6 +962,7 @@ static ssize_t ffs_epfile_aio_write(struct kiocb *kiocb,
unsigned long nr_segs, loff_t loff)
{
struct ffs_io_data *io_data;
+ ssize_t res;
ENTER();
@@ -981,7 +982,10 @@ static ssize_t ffs_epfile_aio_write(struct kiocb *kiocb,
kiocb_set_cancel_fn(kiocb, ffs_aio_cancel);
- return ffs_epfile_io(kiocb->ki_filp, io_data);
+ res = ffs_epfile_io(kiocb->ki_filp, io_data);
+ if (res != -EIOCBQUEUED)
+ kfree(io_data);
+ return res;
}
static ssize_t ffs_epfile_aio_read(struct kiocb *kiocb,
@@ -990,6 +994,7 @@ static ssize_t ffs_epfile_aio_read(struct kiocb *kiocb,
{
struct ffs_io_data *io_data;
struct iovec *iovec_copy;
+ ssize_t res;
ENTER();
@@ -1017,7 +1022,12 @@ static ssize_t ffs_epfile_aio_read(struct kiocb *kiocb,
kiocb_set_cancel_fn(kiocb, ffs_aio_cancel);
- return ffs_epfile_io(kiocb->ki_filp, io_data);
+ res = ffs_epfile_io(kiocb->ki_filp, io_data);
+ if (res != -EIOCBQUEUED) {
+ kfree(io_data);
+ kfree(iovec_copy);
+ }
+ return res;
}
static int
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/6] new helper: dup_iter()
From: Al Viro
Copy iter and kmemdup the underlying array for the copy. Returns
a pointer to result of kmemdup() to be kfree()'d later.
Signed-off-by: Al Viro
---
include/linux/uio.h | 2 ++
mm/iov_iter.c | 15 +++
2 files changed, 17 insertions(+)
diff --git a/include/linux/uio.h b/include/linux/uio.h
index b447402..e325cb1 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -98,6 +98,8 @@ ssize_t iov_iter_get_pages_alloc(struct iov_iter *i, struct
page ***pages,
size_t maxsize, size_t *start);
int iov_iter_npages(const struct iov_iter *i, int maxpages);
+const void *dup_iter(struct iov_iter *new, struct iov_iter *old, gfp_t flags);
+
static inline size_t iov_iter_count(struct iov_iter *i)
{
return i->count;
diff --git a/mm/iov_iter.c b/mm/iov_iter.c
index 8277320..9d96e283 100644
--- a/mm/iov_iter.c
+++ b/mm/iov_iter.c
@@ -751,3 +751,18 @@ int iov_iter_npages(const struct iov_iter *i, int maxpages)
return npages;
}
EXPORT_SYMBOL(iov_iter_npages);
+
+const void *dup_iter(struct iov_iter *new, struct iov_iter *old, gfp_t flags)
+{
+ *new = *old;
+ if (new->type & ITER_BVEC)
+ return new->bvec = kmemdup(new->bvec,
+ new->nr_segs * sizeof(struct bio_vec),
+ flags);
+ else
+ /* iovec and kvec have identical layout */
+ return new->iov = kmemdup(new->iov,
+ new->nr_segs * sizeof(struct iovec),
+ flags);
+}
+EXPORT_SYMBOL(dup_iter);
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/6] gadget/function/f_fs.c: switch to ->{read,write}_iter()
From: Al Viro
Signed-off-by: Al Viro
---
drivers/usb/gadget/function/f_fs.c | 136 -
1 file changed, 58 insertions(+), 78 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index 11704e7..04c6542 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -856,38 +856,6 @@ error:
return ret;
}
-static ssize_t
-ffs_epfile_write(struct file *file, const char __user *buf, size_t len,
-loff_t *ptr)
-{
- struct ffs_io_data io_data;
- struct iovec iov = {.iov_base = buf, .iov_len = len};
-
- ENTER();
-
- io_data.aio = false;
- io_data.read = false;
- iov_iter_init(&io_data.data, WRITE, &iov, 1, len);
-
- return ffs_epfile_io(file, &io_data);
-}
-
-static ssize_t
-ffs_epfile_read(struct file *file, char __user *buf, size_t len, loff_t *ptr)
-{
- struct ffs_io_data io_data;
- struct iovec iov = {.iov_base = buf, .iov_len = len};
-
- ENTER();
-
- io_data.aio = false;
- io_data.read = true;
- io_data.to_free = NULL;
- iov_iter_init(&io_data.data, READ, &iov, 1, len);
-
- return ffs_epfile_io(file, &io_data);
-}
-
static int
ffs_epfile_open(struct inode *inode, struct file *file)
{
@@ -924,72 +892,84 @@ static int ffs_aio_cancel(struct kiocb *kiocb)
return value;
}
-static ssize_t ffs_epfile_aio_write(struct kiocb *kiocb,
- const struct iovec *iovec,
- unsigned long nr_segs, loff_t loff)
+static ssize_t ffs_epfile_write_iter(struct kiocb *kiocb, struct iov_iter
*from)
{
- struct ffs_io_data *io_data;
+ struct ffs_io_data io_data, *p = &io_data;
ssize_t res;
ENTER();
- io_data = kmalloc(sizeof(*io_data), GFP_KERNEL);
- if (unlikely(!io_data))
- return -ENOMEM;
+ if (!is_sync_kiocb(kiocb)) {
+ p = kmalloc(sizeof(io_data), GFP_KERNEL);
+ if (unlikely(!p))
+ return -ENOMEM;
+ p->aio = true;
+ } else {
+ p->aio = false;
+ }
- io_data->aio = true;
- io_data->read = false;
- io_data->kiocb = kiocb;
- iov_iter_init(&io_data->data, WRITE, iovec, nr_segs, kiocb->ki_nbytes);
- io_data->mm = current->mm;
+ p->read = false;
+ p->kiocb = kiocb;
+ p->data = *from;
+ p->mm = current->mm;
- kiocb->private = io_data;
+ kiocb->private = p;
kiocb_set_cancel_fn(kiocb, ffs_aio_cancel);
- res = ffs_epfile_io(kiocb->ki_filp, io_data);
- if (res != -EIOCBQUEUED)
- kfree(io_data);
+ res = ffs_epfile_io(kiocb->ki_filp, p);
+ if (res == -EIOCBQUEUED)
+ return res;
+ if (p->aio)
+ kfree(p);
+ else
+ *from = p->data;
return res;
}
-static ssize_t ffs_epfile_aio_read(struct kiocb *kiocb,
- const struct iovec *iovec,
- unsigned long nr_segs, loff_t loff)
+static ssize_t ffs_epfile_read_iter(struct kiocb *kiocb, struct iov_iter *to)
{
- struct ffs_io_data *io_data;
- struct iovec *iovec_copy;
+ struct ffs_io_data io_data, *p = &io_data;
ssize_t res;
ENTER();
- iovec_copy = kmalloc_array(nr_segs, sizeof(*iovec_copy), GFP_KERNEL);
- if (unlikely(!iovec_copy))
- return -ENOMEM;
-
- memcpy(iovec_copy, iovec, sizeof(struct iovec)*nr_segs);
-
- io_data = kmalloc(sizeof(*io_data), GFP_KERNEL);
- if (unlikely(!io_data)) {
- kfree(iovec_copy);
- return -ENOMEM;
+ if (!is_sync_kiocb(kiocb)) {
+ p = kmalloc(sizeof(io_data), GFP_KERNEL);
+ if (unlikely(!p))
+ return -ENOMEM;
+ p->aio = true;
+ } else {
+ p->aio = false;
}
- io_data->aio = true;
- io_data->read = true;
- io_data->kiocb = kiocb;
- io_data->to_free = iovec_copy;
- iov_iter_init(&io_data->data, READ, iovec_copy, nr_segs,
kiocb->ki_nbytes);
- io_data->mm = current->mm;
+ p->read = true;
+ p->kiocb = kiocb;
+ if (p->aio) {
+ p->to_free = dup_iter(&p->data, to, GFP_KERNEL);
+ if (!p->to_free) {
+ kfree(p);
+ return -ENOMEM;
+ }
+ } else {
+ p->data = *to;
+ p->to_free = NULL;
+ }
+ p->mm = current->mm;
- kiocb->private = io_data;
+ kiocb->private = p;
kiocb_set_cancel_fn(kiocb, ffs_aio_cancel);
-
[PATCH 3/6] gadget/function/f_fs.c: use put iov_iter into io_data
From: Al Viro
both on aio and non-aio sides
Signed-off-by: Al Viro
---
drivers/usb/gadget/function/f_fs.c | 86 +++---
1 file changed, 25 insertions(+), 61 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index 0c120ad..11704e7 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -143,10 +143,9 @@ struct ffs_io_data {
bool read;
struct kiocb *kiocb;
- const struct iovec *iovec;
- unsigned long nr_segs;
- char __user *buf;
- size_t len;
+ struct iov_iter data;
+ const void *to_free;
+ char *buf;
struct mm_struct *mm;
struct work_struct work;
@@ -645,29 +644,10 @@ static void ffs_user_copy_worker(struct work_struct *work)
io_data->req->actual;
if (io_data->read && ret > 0) {
- int i;
- size_t pos = 0;
-
- /*
-* Since req->length may be bigger than io_data->len (after
-* being rounded up to maxpacketsize), we may end up with more
-* data then user space has space for.
-*/
- ret = min_t(int, ret, io_data->len);
-
use_mm(io_data->mm);
- for (i = 0; i < io_data->nr_segs; i++) {
- size_t len = min_t(size_t, ret - pos,
- io_data->iovec[i].iov_len);
- if (!len)
- break;
- if (unlikely(copy_to_user(io_data->iovec[i].iov_base,
-&io_data->buf[pos], len))) {
- ret = -EFAULT;
- break;
- }
- pos += len;
- }
+ ret = copy_to_iter(io_data->buf, ret, &io_data->data);
+ if (iov_iter_count(&io_data->data))
+ ret = -EFAULT;
unuse_mm(io_data->mm);
}
@@ -677,7 +657,7 @@ static void ffs_user_copy_worker(struct work_struct *work)
io_data->kiocb->private = NULL;
if (io_data->read)
- kfree(io_data->iovec);
+ kfree(io_data->to_free);
kfree(io_data->buf);
kfree(io_data);
}
@@ -736,6 +716,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct
ffs_io_data *io_data)
* before the waiting completes, so do not assign to 'gadget'
earlier
*/
struct usb_gadget *gadget = epfile->ffs->gadget;
+ size_t copied;
spin_lock_irq(&epfile->ffs->eps_lock);
/* In the meantime, endpoint got disabled or changed. */
@@ -743,34 +724,21 @@ static ssize_t ffs_epfile_io(struct file *file, struct
ffs_io_data *io_data)
spin_unlock_irq(&epfile->ffs->eps_lock);
return -ESHUTDOWN;
}
+ data_len = iov_iter_count(&io_data->data);
/*
* Controller may require buffer size to be aligned to
* maxpacketsize of an out endpoint.
*/
- data_len = io_data->read ?
- usb_ep_align_maybe(gadget, ep->ep, io_data->len) :
- io_data->len;
+ if (io_data->read)
+ data_len = usb_ep_align_maybe(gadget, ep->ep, data_len);
spin_unlock_irq(&epfile->ffs->eps_lock);
data = kmalloc(data_len, GFP_KERNEL);
if (unlikely(!data))
return -ENOMEM;
- if (io_data->aio && !io_data->read) {
- int i;
- size_t pos = 0;
- for (i = 0; i < io_data->nr_segs; i++) {
- if (unlikely(copy_from_user(&data[pos],
-io_data->iovec[i].iov_base,
-io_data->iovec[i].iov_len))) {
- ret = -EFAULT;
- goto error;
- }
- pos += io_data->iovec[i].iov_len;
- }
- } else {
- if (!io_data->read &&
- unlikely(__copy_from_user(data, io_data->buf,
- io_data->len))) {
+ if (!io_data->read) {
+ copied = copy_from_iter(data, data_len, &io_data->data);
+ if (copied
[PATCH 5/6] gadgetfs: use-after-free in ->aio_read()
From: Al Viro
AIO_PREAD requests call ->aio_read() with iovec on caller's stack, so if
we are going to access it asynchronously, we'd better get ourselves
a copy - the one on kernel stack of aio_run_iocb() won't be there
anymore. function/f_fs.c take care of doing that, legacy/inode.c
doesn't...
Cc: sta...@vger.kernel.org
Signed-off-by: Al Viro
---
drivers/usb/gadget/legacy/inode.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/gadget/legacy/inode.c
b/drivers/usb/gadget/legacy/inode.c
index db49ec4..9fbbaa0 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -566,7 +566,6 @@ static ssize_t ep_copy_to_user(struct kiocb_priv *priv)
if (total == 0)
break;
}
-
return len;
}
@@ -585,6 +584,7 @@ static void ep_user_copy_worker(struct work_struct *work)
aio_complete(iocb, ret, ret);
kfree(priv->buf);
+ kfree(priv->iv);
kfree(priv);
}
@@ -605,6 +605,7 @@ static void ep_aio_complete(struct usb_ep *ep, struct
usb_request *req)
*/
if (priv->iv == NULL || unlikely(req->actual == 0)) {
kfree(req->buf);
+ kfree(priv->iv);
kfree(priv);
iocb->private = NULL;
/* aio_complete() reports bytes-transferred _and_ faults */
@@ -640,7 +641,7 @@ ep_aio_rwtail(
struct usb_request *req;
ssize_t value;
- priv = kmalloc(sizeof *priv, GFP_KERNEL);
+ priv = kzalloc(sizeof *priv, GFP_KERNEL);
if (!priv) {
value = -ENOMEM;
fail:
@@ -649,7 +650,14 @@ fail:
}
iocb->private = priv;
priv->iocb = iocb;
- priv->iv = iv;
+ if (iv) {
+ priv->iv = kmemdup(iv, nr_segs * sizeof(struct iovec),
+ GFP_KERNEL);
+ if (!priv->iv) {
+ kfree(priv);
+ goto fail;
+ }
+ }
priv->nr_segs = nr_segs;
INIT_WORK(&priv->work, ep_user_copy_worker);
@@ -689,6 +697,7 @@ fail:
mutex_unlock(&epdata->lock);
if (unlikely(value)) {
+ kfree(priv->iv);
kfree(priv);
put_ep(epdata);
} else
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] fs: remove ki_nbytes
On Thu, Feb 05, 2015 at 10:29:42AM -0500, Alan Stern wrote:
> On Wed, 4 Feb 2015, Al Viro wrote:
>
> > > > Um... readv() is also going through ->aio_read().
> > >
> > > Why does readv() do this but not read()? Wouldn't it make more sense
> > > to have all the read* calls use the same internal interface?
> >
> > Because there are two partially overlapping classes wrt vector IO semantics:
> ...
>
> Thanks for the detailed explanation. It appears to boil down to a
> series of historical accidents.
>
> In any case, feel free to copy the non-isochronous behavior of the
> synchronous routines in the async routines. It certainly won't hurt
> anything.
OK, I've limited it to sync ones, actually. Preliminary series in followups.
Those who prefer to read in git, see
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #gadget
WARNING: completely untested
Al Viro (6):
new helper: dup_iter()
gadget/function/f_fs.c: close leaks
gadget/function/f_fs.c: use put iov_iter into io_data
gadget/function/f_fs.c: switch to ->{read,write}_iter()
gadgetfs: use-after-free in ->aio_read()
gadget: switch ep_io_operations to ->read_iter/->write_iter
drivers/usb/gadget/function/f_fs.c | 204 +-
drivers/usb/gadget/legacy/inode.c | 346 +++--
include/linux/uio.h| 2 +
mm/iov_iter.c | 15 ++
4 files changed, 237 insertions(+), 330 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] fs: remove ki_nbytes
On Thu, Feb 05, 2015 at 10:29:42AM -0500, Alan Stern wrote:
> On Wed, 4 Feb 2015, Al Viro wrote:
>
> > > > Um... readv() is also going through ->aio_read().
> > >
> > > Why does readv() do this but not read()? Wouldn't it make more sense
> > > to have all the read* calls use the same internal interface?
> >
> > Because there are two partially overlapping classes wrt vector IO semantics:
> ...
>
> Thanks for the detailed explanation. It appears to boil down to a
> series of historical accidents.
>
> In any case, feel free to copy the non-isochronous behavior of the
> synchronous routines in the async routines. It certainly won't hurt
> anything.
Hmm... What happens if f_fs.c successfully queues struct usb_request, returns
-EIOCBQUEUED and then gets hit by io_cancel(2)? AFAICS, you get
ffs_aio_cancel() called, which dequeues usb_request and buggers off.
The thing is, freeing io_data and stuff hanging off it would be done by
ffs_user_copy_worker(), which would be scheduled via schedule_work() by
ffs_epfile_async_io_complete(), i.e. usb_request ->complete() callback.
And usb_ep_dequeue() (aka. ep->ops->dequeue()) has tons of instances, but
AFAICS some of them might not trigger usb_gadget_giveback_request(), which
would normally call ->complete()...
Example:
net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
struct net2272_ep *ep;
struct net2272_request *req;
unsigned long flags;
int stopped;
ep = container_of(_ep, struct net2272_ep, ep);
if (!_ep || (!ep->desc && ep->num != 0) || !_req)
return -EINVAL;
spin_lock_irqsave(&ep->dev->lock, flags);
stopped = ep->stopped;
ep->stopped = 1;
/* make sure it's still queued on this endpoint */
list_for_each_entry(req, &ep->queue, queue) {
if (&req->req == _req)
break;
}
if (&req->req != _req) {
spin_unlock_irqrestore(&ep->dev->lock, flags);
return -EINVAL;
}
/* queue head may be partially complete */
if (ep->queue.next == &req->queue) {
dev_dbg(ep->dev->dev, "unlink (%s) pio\n", _ep->name);
net2272_done(ep, req, -ECONNRESET);
}
req = NULL;
ep->stopped = stopped;
spin_unlock_irqrestore(&ep->dev->lock, flags);
return 0;
}
Note that net2272_done(), which would call usb_gadget_giveback_request(),
is only called if the victim happens to be queue head. Is that just a
net2272.c bug, or am I missing something subtle here? Looks like
at least on that hardware io_cancel() could leak io_data and everything
that hangs off it...
FWIW, net2272.c was the first one I looked at (happened to be on the last
line of screen during git grep for \.dequeue in drivers/usb/gadget ;-)
and after checking several more it seems that it's a Sod's Law in action -
I'd checked about 5 of them and they seem to call usb_gadget_giveback_request()
as long as they find the sucker in queue, head or no head. OTOH, there's
a lot more of those guys, so that observation is not worth much...
IOW, is that a net2272.c bug, or a f_fs.c one?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] fs: remove ki_nbytes
On Thu, Feb 05, 2015 at 08:47:29AM +, Al Viro wrote: > You are confusing datagram-per-syscall (which they are) with > datagram-per-iovec (which they are definitely not). IOW, they behave > as UDP sockets - writev() is purely scatter-gather variant of write(), > with datagram per syscall and all vector elements silently concatenated. > That's class 2, and _not_ in its intersection with class 1. PS: you want class 1, look at something like /proc/sys/kernel/domainname (or any other sysctl of that sort). write "foobar" there and cat /proc/sys/kernel/domainname will print foorbat. writev an array consisting of "foo" and "bar", and you'll see bar afterwards, same as you would after writing first "foo", then "bar". There the iovec boundaries affect the result - ->no aio_write() for that sucker, so we get two calls of ->write(), with expected results. And there are character devices like that as well. _That_ is class 1 outside of intersection with class 2. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] fs: remove ki_nbytes
On Thu, Feb 05, 2015 at 09:24:32AM +0100, Robert Baldyga wrote:
> No, function/f_fs.c and legacy/inode.c are in class (1). They have
> datagram semantics - each vector element is submitted in separate USB
> request. Each single request is sent in separate USB data packet (for
> bulk endpoints it can be more than one packet). In fact sync
> read()/write() also will give different results while called once with
> some block of data or in loop with the same block of data splitted into
> a few parts.
No, they don't. This is from ffs_epfile_io():
data = kmalloc(data_len, GFP_KERNEL);
if (unlikely(!data))
return -ENOMEM;
if (io_data->aio && !io_data->read) {
int i;
size_t pos = 0;
for (i = 0; i < io_data->nr_segs; i++) {
if (unlikely(copy_from_user(&data[pos],
io_data->iovec[i].iov_base,
io_data->iovec[i].iov_len))) {
ret = -EFAULT;
goto error;
}
pos += io_data->iovec[i].iov_len;
}
and that's the last point where it looks at iovec. After that all work
is done to the copy in data, where no information about the boundaries
survives. And ep_aio_write() (in legacy/inode.c) is the same way.
You are confusing datagram-per-syscall (which they are) with
datagram-per-iovec (which they are definitely not). IOW, they behave
as UDP sockets - writev() is purely scatter-gather variant of write(),
with datagram per syscall and all vector elements silently concatenated.
That's class 2, and _not_ in its intersection with class 1.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] fs: remove ki_nbytes
On Wed, Feb 04, 2015 at 03:30:32PM -0500, Alan Stern wrote:
> > * this one. Note that you are not guaranteed that ep_config() won't
> > be called more than once - two threads might race in write(2), with the
> > loser
> > getting through mutex_lock_interruptible(&data->lock); in ep_config() only
> > after the winner has already gotten through write(), switched ->f_op,
> > returned
> > to userland and started doing read()/write()/etc. If nothing else,
> > the contents of data->desc and data->hs_desc can be buggered by arbitrary
> > data, no matter how bogus, right as the first thread is doing IO.
>
> Well, this one certainly can be fixed to avoid altering ->f_op, at the
> cost of adding an extra check at the start of each I/O operation.
> > Um... readv() is also going through ->aio_read().
>
> Why does readv() do this but not read()? Wouldn't it make more sense
> to have all the read* calls use the same internal interface?
Because there are two partially overlapping classes wrt vector IO semantics:
1) datagram-style. Vectored read/write is equivalent to simple
read/write done on each vector component. And IO boundaries matter - if
your driver treats any write() as datagram that starts e.g. with
fixed-sized table in the beginning + arbitrary amount of data following
it, you will get very different results from write(fd, buf, 200) and
writev(fd, (struct iovec[2]){{buf, 100}, {buf+100, 100}}, 2). A _lot_ of
drivers are like that - they supply ->read() and ->write() for single-range
IO and VFS construct the rest of operations out of those.
2) stream-style. Vectored read is guaranteed to behave the same
way as simple read on a range with size being the sum of vector element
sizes, except that the data ends in ranges covered by vector elements instead
of a single array. Vectored write is guaranteed to behave the same way
as simple write from a buffer containing the concatenation of ranges covered
by vector elements. Boundaries between the elements do not matter at all.
Regular files on storage filesystems are like that. So are FIFOs and pipes
and so are sockets. Even for datagram protocols, boundaries between the
vector elements are ignored; boundaries between syscalls provide the datagram
boundaries, but you can e.g. do writev(udp_socket_fd, (struct iovec[3]){
{const_header, sizeof(const_header)}, {&n, 4}, {s, strlen(s)}}, 3) and have
only one UDP packet sent. IOW, it's general-purpose scatter-gather for read
and write.
The last example shows that (2) isn't a subset of (1) - it's not
always possible to call ->write() in loop and get the right behaviour.
For regular files (and pure stream sockets, etc.) it would work, but for
stuff like UDP sockets it would break. Moreover, even for regular files on
storage filesystems it would be quite inefficient - we'd need to acquire and
release a bunch of locks, poke through metadata, etc., for each segment.
As the result, there was a couple of new methods added, inventively
called ->readv() and ->writev(). do_sync_read() was supposed to be used
as ->read() instance - it's "feed a single-element vector to ->readv()" and
similar for s/read/write/.
Note that both (1) and (2) satisfy the following sanity requirement -
single-element readv() is always equivalent to single-element() read(). You
could violate that, by providing completely unrelated ->read() and ->readv(),
but very few drivers went for that - too insane.
Then, when AIO had been added, those had grown an argument pointing
to iocb (instead of file and ppos - for those we use iocb->ki_filp and
&iocb->ki_pos resp.) and they got renamed into ->aio_read() and ->aio_write().
Note that non-vectored AIO uses the same methods - ->read() and ->write() had
too many instances to convert and most of those would end up just using those
two iocb fields instead of the old arguments - tons of churn for no good
reason. ->readv() and ->writev() had fewer instances (very common was the
use of generic_file_aio_{read,write}()) and conversion was less painful.
So there was no ->aio_read() and ->aio_write(). That, in principle, was a
bit of constraint - you couldn't make single-element AIO_PREADV behave
different from AIO_PREAD, but nobody had been insane enough to ask for that.
Moreover, keeping ->readv() and ->writev() was pointless. There is
cheap way to tell whether ->aio_{read,write}() call is due to io_submit(2)
or to readv()/writev() - is_sync_kiocb(iocb) tells which one it is, so if
driver really wanted different semantics for sync vs. async, it could check
that.
So we ended up with ->read/->write for sync non-vectored and
->aio_read()/->aio_write() for sync vectored *and* async anything. Usually
you provide one or the other - NULL ->aio_... means loop calling ->read/write
on each element, NULL ->read/write (or do_sync_... for them - it's the same
thing) means feeding sync iocb and single-element vector to ->aio_
You *can* pro
Re: [PATCH 3/5] fs: remove ki_nbytes
On Wed, Feb 04, 2015 at 01:17:30PM -0500, Alan Stern wrote: > 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. Yecchhh... Anything that changes ->f_op *after* return from ->open() is doing a nasty, nasty thing. What's to guarantee that any checks for NULL fields will stay valid, etc.? FWIW, in all the tree there are only 4 places where that would be happening; * i810_map_buffer() screwing around with having vm_mmap() done, only it wants its own thing called as ->mmap() (and a bit of extra data stashed for it). Racy as hell (if another thread calls mmap() on the same file, you'll get a nasty surprise). Driver's too old and brittle to touch, according to drm folks... * TTY hangup logics. Nasty (and might be broken around ->fasync()), but it's a very special case. * snd_card_disconnect(). Analogue of TTY hangup, actually; both are trying to do a form of revoke(). * this one. Note that you are not guaranteed that ep_config() won't be called more than once - two threads might race in write(2), with the loser getting through mutex_lock_interruptible(&data->lock); in ep_config() only after the winner has already gotten through write(), switched ->f_op, returned to userland and started doing read()/write()/etc. If nothing else, the contents of data->desc and data->hs_desc can be buggered by arbitrary data, no matter how bogus, right as the first thread is doing IO. > > [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? Yes. > > 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. Yes, but you have readv() on single-element vector behave different from read(), which is surprising, to put it mildly. > > 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. Um... readv() is also going through ->aio_read(). I can tie that to sync vs. async, though - is_sync_kiocb() will do just that, if you are OK with having readv() act the same as read() in that respect. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] fs: remove ki_nbytes
[USB folks Cc'd]
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; 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]
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. 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.
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...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] usb: ffs: fix regression when quirk_ep_out_aligned_size flag is set
On Wed, Oct 08, 2014 at 02:12:18PM -0700, David Cohen wrote:
> use_mm(io_data->mm);
> for (i = 0; i < io_data->nr_segs; i++) {
> + size_t len = min_t(size_t, ret - pos,
> + io_data->iovec[i].iov_len);
> + if (!len)
> + break;
> if (unlikely(copy_to_user(io_data->iovec[i].iov_base,
> - &io_data->buf[pos],
> - io_data->iovec[i].iov_len))) {
> + &io_data->buf[pos], len))) {
> ret = -EFAULT;
> break;
> }
> - pos += io_data->iovec[i].iov_len;
> + pos += len;
Hmm... This is really asking for something like
if (copy_to_iter(io_data->buf, ret, ) != ret)
ret = -EFAULT;
with being an iov_iter instead of iovec. It would be really
nice to have that thing switched to ->read_iter/->write_iter, dropping
->read/->write/->aio_read/->aio_write; I'm not familiar enough with that
code to do it on my own, though, so it would require some help from
maintainers...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][RFC] Fix breakage in ffs_fs_mount()
On Sat, Sep 21, 2013 at 01:10:48AM +0200, Michal Nazarewicz wrote: > On Fri, Sep 20 2013, Al Viro wrote: > > There's a bunch of failure exits in ffs_fs_mount() with > > seriously broken recovery logics. Most of that appears to stem > > from misunderstanding of the ->kill_sb() semantics; > > That sounds likely. > > [???] > > > Signed-off-by: Al Viro > > Acked-by: Michal Nazarewicz Umm... Which tree should that go through? I can put it in vfs.git, of course, but it's local to drivers/usb/gadget, so a usb tree might be a better fit... It's -stable fodder, all way back to linux-3.3.y. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH][RFC] Fix breakage in ffs_fs_mount()
There's a bunch of failure exits in ffs_fs_mount() with
seriously broken recovery logics. Most of that appears to stem
from misunderstanding of the ->kill_sb() semantics; unlike
->put_super() it is called for *all* superblocks of given type,
no matter how (in)complete the setup had been. ->put_super()
is called only if ->s_root is not NULL; any failure prior to
setting ->s_root will have the call of ->put_super() skipped.
->kill_sb(), OTOH, awaits every superblock that has come from
sget().
Current behaviour of ffs_fs_mount():
We have struct ffs_sb_fill_data data on stack there. We do
ffs_dev = functionfs_acquire_dev_callback(dev_name);
and store that in data.private_data. Then we call mount_nodev(),
passing it ffs_sb_fill() as a callback. That will either fail
outright, or manage to call ffs_sb_fill(). There we allocate an
instance of struct ffs_data, slap the value of ffs_dev (picked
from data.private_data) into ffs->private_data and overwrite
data.private_data by storing ffs into an overlapping member
(data.ffs_data). Then we store ffs into sb->s_fs_info and attempt
to set the rest of the things up (root inode, root dentry, then
create /ep0 there). Any of those might fail. Should that
happen, we get ffs_fs_kill_sb() called before mount_nodev()
returns. If mount_nodev() fails for any reason whatsoever,
we proceed to
functionfs_release_dev_callback(data.ffs_data);
That's broken in a lot of ways. Suppose the thing has failed in
allocation of e.g. root inode or dentry. We have
functionfs_release_dev_callback(ffs);
ffs_data_put(ffs);
done by ffs_fs_kill_sb() (ffs accessed via sb->s_fs_info), followed by
functionfs_release_dev_callback(ffs);
from ffs_fs_mount() (via data.ffs_data). Note that the second
functionfs_release_dev_callback() has every chance to be done to freed memory.
Suppose we fail *before* root inode allocation. What happens then?
ffs_fs_kill_sb() doesn't do anything to ffs (it's either not called at all,
or it doesn't have a pointer to ffs stored in sb->s_fs_info). And
functionfs_release_dev_callback(data.ffs_data);
is called by ffs_fs_mount(), but here we are in nasal daemon country - we
are reading from a member of union we'd never stored into. In practice,
we'll get what we used to store into the overlapping field, i.e. ffs_dev.
And then we get screwed, since we treat it (struct gfs_ffs_obj * in
disguise, returned by functionfs_acquire_dev_callback()) as struct
ffs_data *, pick what would've been ffs_data ->private_data from it
(*well* past the actual end of the struct gfs_ffs_obj - struct ffs_data
is much bigger) and poke in whatever it points to.
FWIW, there's a minor leak on top of all that in case if ffs_sb_fill()
fails on kstrdup() - ffs is obviously forgotten.
The thing is, there is no point in playing all those games with union.
Just allocate and initialize ffs_data *before* calling mount_nodev() and
pass a pointer to it via data.ffs_data. And once it's stored in
sb->s_fs_info, clear data.ffs_data, so that ffs_fs_mount() knows that
it doesn't need to kill the sucker manually - from that point on
we'll have it done by ->kill_sb().
Signed-off-by: Al Viro
--
diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 1a66c5b..0658908 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -1034,37 +1034,19 @@ struct ffs_sb_fill_data {
struct ffs_file_perms perms;
umode_t root_mode;
const char *dev_name;
- union {
- /* set by ffs_fs_mount(), read by ffs_sb_fill() */
- void *private_data;
- /* set by ffs_sb_fill(), read by ffs_fs_mount */
- struct ffs_data *ffs_data;
- };
+ struct ffs_data *ffs_data;
};
static int ffs_sb_fill(struct super_block *sb, void *_data, int silent)
{
struct ffs_sb_fill_data *data = _data;
struct inode*inode;
- struct ffs_data *ffs;
+ struct ffs_data *ffs = data->ffs_data;
ENTER();
- /* Initialise data */
- ffs = ffs_data_new();
- if (unlikely(!ffs))
- goto Enomem;
-
ffs->sb = sb;
- ffs->dev_name= kstrdup(data->dev_name, GFP_KERNEL);
- if (unlikely(!ffs->dev_name))
- goto Enomem;
- ffs->file_perms = data->perms;
- ffs->private_data= data->private_data;
-
- /* used by the caller of this function */
- data->ffs_data = ffs;
-
+ data->ffs_data = NULL;
sb->s_fs_info= ffs;
sb->s_blocksize = PAGE_CACHE_SIZE;
sb->s_blocksize_bits = PAGE_CACHE_SHIFT;
@@ -1080,17 +1062,14 @@ static int ffs_sb_fill(struct super_block *sb, void
*_data, int silent)
&d
