[git pull] gadgetfs fixes

[git pull] gadgetfs fixes

[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()
      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%)

Re: [git pull] gadgetfs fixes

[Alexander Holler]

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.

Alexander Holler

Re: [git pull] gadgetfs fixes

[Al Viro]

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...

Re: [git pull] gadgetfs fixes

[Alexander Holler]

Am 15.03.2015 um 02:39 schrieb Al Viro:
> 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

Glücklicherweise nicht. Vielleicht sollten wir es zur Abwechslung mal
mit meiner bevorzugten Sprache versuchen.

> 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...

The following patches aren't marked for stable, otherwise I would not
have risked to become a victim of your comments again.

Alexander Holler
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [git pull] gadgetfs fixes

[Al Viro]

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...

Re: [git pull] gadgetfs fixes

[Alexander Holler]

Am 15.03.2015 um 09:17 schrieb Al Viro:
> 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...

I haven't tried to apply the whole series to the 3.19.1 which I'm 
currently using. As mentioned before, something (a single patch I've 
tried before) didn't apply cleanly which means I need to have a deeper 
look at the stuff. E.g. I've just learned (through another problem), 
that (my version of) glibc still doesn't use the aio-syscalls the kernel 
provides (and instead uses pread/pwrite).

I will look if I find the time today.


--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [git pull] gadgetfs fixes

[Alexander Holler]

Am 15.03.2015 um 09:31 schrieb Alexander Holler:
> Am 15.03.2015 um 09:17 schrieb Al Viro:
>> 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...
>
> I haven't tried to apply the whole series to the 3.19.1 which I'm
> currently using. As mentioned before, something (a single patch I've
> tried before) didn't apply cleanly which means I need to have a deeper
> look at the stuff. E.g. I've just learned (through another problem),
> that (my version of) glibc still doesn't use the aio-syscalls the kernel
> provides (and instead uses pread/pwrite).
>
> I will look if I find the time today.

Also it's already merged, looks good. Please just don't forget to add that

value = -ENOMEM;

if commit f01d35a1 goes down the road without the following patches.

Thanks,

Alexander Holler


--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [git pull] gadgetfs fixes

[Alexander Holler]

Am 15.03.2015 um 09:17 schrieb Al Viro:

> 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...

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.

Alexander Holler

Re: [git pull] gadgetfs fixes

[Al Viro]

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.