[PATCH] fuse: Fix IOC_[GS]ETFLAGS argument size brokenness

[PATCH] fuse: Fix IOC_[GS]ETFLAGS argument size brokenness

[Darrick J. Wong]

The IOC_[GS]ETFLAGS ioctls, despite being defined to take a "long"
parameter, actually take "int" parameters.  FUSE unfortunately assumed
that the ioctl definitions never lie, and transfers a long's worth of
data in and out of userspace, which causes stack smashing in chattr,
and other bugs elsewhere.

So, special-case this in FUSE, and document this int/long quirk in
include/uapi/linux/fs.h.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/fuse/file.c          |   11 +++++++++++
 include/uapi/linux/fs.h |    1 +
 2 files changed, 12 insertions(+)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 7e70506..5fa8181 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2385,6 +2385,17 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg,
 		iov->iov_base = (void __user *)arg;
 		iov->iov_len = _IOC_SIZE(cmd);
 
+		/*
+		 * The IOC_[GS]ETFLAGS ioctls take int parameters even though
+		 * the ioctl definition specifies long.  Userland has been
+		 * expecting int for ages (and chattr segfaults on FUSE
+		 * filesystems), so special case that here.  The IOC32
+		 * variants were declared with int, so they don't need this.
+		 */
+		if (cmd == FS_IOC_GETFLAGS || cmd == FS_IOC_SETFLAGS) {
+			iov->iov_len = sizeof(int);
+		}
+
 		if (_IOC_DIR(cmd) & _IOC_WRITE) {
 			in_iov = iov;
 			in_iovs = 1;
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 6c28b61..bc8aa8e 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -154,6 +154,7 @@ struct inodes_stat_t {
 #define FITHAW		_IOWR('X', 120, int)	/* Thaw */
 #define FITRIM		_IOWR('X', 121, struct fstrim_range)	/* Trim */
 
+/* IOC_[GS]ETFLAGS take an int argument despite being defined to take long. */
 #define	FS_IOC_GETFLAGS			_IOR('f', 1, long)
 #define	FS_IOC_SETFLAGS			_IOW('f', 2, long)
 #define	FS_IOC_GETVERSION		_IOR('v', 1, long)

Re: [PATCH] fuse: Fix IOC_[GS]ETFLAGS argument size brokenness

[Richard Hansen]

On 2013-12-19 18:27, Darrick J. Wong wrote:
> The IOC_[GS]ETFLAGS ioctls, despite being defined to take a "long"
> parameter, actually take "int" parameters.  FUSE unfortunately assumed
> that the ioctl definitions never lie, and transfers a long's worth of
> data in and out of userspace, which causes stack smashing in chattr,
> and other bugs elsewhere.
> 
> So, special-case this in FUSE, and document this int/long quirk in
> include/uapi/linux/fs.h.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/fuse/file.c          |   11 +++++++++++
>  include/uapi/linux/fs.h |    1 +
>  2 files changed, 12 insertions(+)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 7e70506..5fa8181 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -2385,6 +2385,17 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg,
>  		iov->iov_base = (void __user *)arg;
>  		iov->iov_len = _IOC_SIZE(cmd);
>  
> +		/*
> +		 * The IOC_[GS]ETFLAGS ioctls take int parameters even though
> +		 * the ioctl definition specifies long.  Userland has been
> +		 * expecting int for ages (and chattr segfaults on FUSE
> +		 * filesystems), so special case that here.  The IOC32
> +		 * variants were declared with int, so they don't need this.
> +		 */
> +		if (cmd == FS_IOC_GETFLAGS || cmd == FS_IOC_SETFLAGS) {
> +			iov->iov_len = sizeof(int);
> +		}
> +
>  		if (_IOC_DIR(cmd) & _IOC_WRITE) {
>  			in_iov = iov;
>  			in_iovs = 1;
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 6c28b61..bc8aa8e 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -154,6 +154,7 @@ struct inodes_stat_t {
>  #define FITHAW		_IOWR('X', 120, int)	/* Thaw */
>  #define FITRIM		_IOWR('X', 121, struct fstrim_range)	/* Trim */
>  
> +/* IOC_[GS]ETFLAGS take an int argument despite being defined to take long. */
>  #define	FS_IOC_GETFLAGS			_IOR('f', 1, long)
>  #define	FS_IOC_SETFLAGS			_IOW('f', 2, long)
>  #define	FS_IOC_GETVERSION		_IOR('v', 1, long)

This change to include/uapi/linux/fs.h is similar to but not as thorough as:

http://mid.gmane.org/1385548839-16617-1-git-send-email-aurelien@aurel32.net
https://lkml.org/lkml/2013/11/27/93

(The above linked patch is the patch Aurelien referenced in the
"Argument type for FS_IOC_GETFLAGS/FS_IOC_SETFLAGS ioctls" thread on
linux-fsdevel; see
<http://mid.gmane.org/20131127231430.GG29912@hall.aurel32.net>.)

-Richard

Re: [PATCH] fuse: Fix IOC_[GS]ETFLAGS argument size brokenness

[Richard Hansen]

On 2013-12-19 18:27, Darrick J. Wong wrote:
> The IOC_[GS]ETFLAGS ioctls, despite being defined to take a "long"
> parameter, actually take "int" parameters.  FUSE unfortunately assumed
> that the ioctl definitions never lie, and transfers a long's worth of
> data in and out of userspace, which causes stack smashing in chattr,
> and other bugs elsewhere.
> 
> So, special-case this in FUSE, and document this int/long quirk in
> include/uapi/linux/fs.h.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/fuse/file.c          |   11 +++++++++++
>  include/uapi/linux/fs.h |    1 +
>  2 files changed, 12 insertions(+)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 7e70506..5fa8181 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -2385,6 +2385,17 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg,
>  		iov->iov_base = (void __user *)arg;
>  		iov->iov_len = _IOC_SIZE(cmd);
>  
> +		/*
> +		 * The IOC_[GS]ETFLAGS ioctls take int parameters even though
> +		 * the ioctl definition specifies long.  Userland has been
> +		 * expecting int for ages (and chattr segfaults on FUSE
> +		 * filesystems), so special case that here.  The IOC32
> +		 * variants were declared with int, so they don't need this.
> +		 */
> +		if (cmd == FS_IOC_GETFLAGS || cmd == FS_IOC_SETFLAGS) {
> +			iov->iov_len = sizeof(int);
> +		}
> +
>  		if (_IOC_DIR(cmd) & _IOC_WRITE) {
>  			in_iov = iov;
>  			in_iovs = 1;

What about FS_IOC_GETVERSION and FS_IOC_SETVERSION?  Should they also be
special-cased?  See:
http://article.gmane.org/gmane.linux.kernel/1602607

-Richard


> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 6c28b61..bc8aa8e 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -154,6 +154,7 @@ struct inodes_stat_t {
>  #define FITHAW		_IOWR('X', 120, int)	/* Thaw */
>  #define FITRIM		_IOWR('X', 121, struct fstrim_range)	/* Trim */
>  
> +/* IOC_[GS]ETFLAGS take an int argument despite being defined to take long. */
>  #define	FS_IOC_GETFLAGS			_IOR('f', 1, long)
>  #define	FS_IOC_SETFLAGS			_IOW('f', 2, long)
>  #define	FS_IOC_GETVERSION		_IOR('v', 1, long)

Re: [PATCH] fuse: Fix IOC_[GS]ETFLAGS argument size brokenness

[Darrick J. Wong]

On Fri, Dec 20, 2013 at 01:19:27AM -0500, Richard Hansen wrote:
> On 2013-12-19 18:27, Darrick J. Wong wrote:
> > The IOC_[GS]ETFLAGS ioctls, despite being defined to take a "long"
> > parameter, actually take "int" parameters.  FUSE unfortunately assumed
> > that the ioctl definitions never lie, and transfers a long's worth of
> > data in and out of userspace, which causes stack smashing in chattr,
> > and other bugs elsewhere.
> > 
> > So, special-case this in FUSE, and document this int/long quirk in
> > include/uapi/linux/fs.h.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/fuse/file.c          |   11 +++++++++++
> >  include/uapi/linux/fs.h |    1 +
> >  2 files changed, 12 insertions(+)
> > 
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index 7e70506..5fa8181 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -2385,6 +2385,17 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg,
> >  		iov->iov_base = (void __user *)arg;
> >  		iov->iov_len = _IOC_SIZE(cmd);
> >  
> > +		/*
> > +		 * The IOC_[GS]ETFLAGS ioctls take int parameters even though
> > +		 * the ioctl definition specifies long.  Userland has been
> > +		 * expecting int for ages (and chattr segfaults on FUSE
> > +		 * filesystems), so special case that here.  The IOC32
> > +		 * variants were declared with int, so they don't need this.
> > +		 */
> > +		if (cmd == FS_IOC_GETFLAGS || cmd == FS_IOC_SETFLAGS) {
> > +			iov->iov_len = sizeof(int);
> > +		}
> > +
> >  		if (_IOC_DIR(cmd) & _IOC_WRITE) {
> >  			in_iov = iov;
> >  			in_iovs = 1;
> > diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> > index 6c28b61..bc8aa8e 100644
> > --- a/include/uapi/linux/fs.h
> > +++ b/include/uapi/linux/fs.h
> > @@ -154,6 +154,7 @@ struct inodes_stat_t {
> >  #define FITHAW		_IOWR('X', 120, int)	/* Thaw */
> >  #define FITRIM		_IOWR('X', 121, struct fstrim_range)	/* Trim */
> >  
> > +/* IOC_[GS]ETFLAGS take an int argument despite being defined to take long. */
> >  #define	FS_IOC_GETFLAGS			_IOR('f', 1, long)
> >  #define	FS_IOC_SETFLAGS			_IOW('f', 2, long)
> >  #define	FS_IOC_GETVERSION		_IOR('v', 1, long)
> 
> This change to include/uapi/linux/fs.h is similar to but not as thorough as:
> 
> http://mid.gmane.org/1385548839-16617-1-git-send-email-aurelien-rXXEIb44qovR7s880joybQ@public.gmane.org
> https://lkml.org/lkml/2013/11/27/93
> 
> (The above linked patch is the patch Aurelien referenced in the
> "Argument type for FS_IOC_GETFLAGS/FS_IOC_SETFLAGS ioctls" thread on
> linux-fsdevel; see
> <http://mid.gmane.org/20131127231430.GG29912-OqXK5JiLQY5aJl8KAwiEcA@public.gmane.org>.)

Aha!  I thought I'd seen something like that on the list, though it doesn't
seem to be in -rc.  Thank you for pointing that out.

And yes, now that I look at all the [GS]ETVERSION implementations, I think FUSE
could special-case those two ioctls too.

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

------------------------------------------------------------------------------
Rapidly troubleshoot problems before they affect your business. Most IT 
organizations don't have a clear picture of how application performance 
affects their revenue. With AppDynamics, you get 100% visibility into your 
Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro!
http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk

[PATCH v2] fuse: Fix IOC_[GS]ET{FLAGS, VERSION} argument size brokenness.

[Darrick J. Wong]

The IOC_[GS]ETFLAGS and IOC_[GS]ETVERSION ioctls, despite being
defined to take a "long" parameter, actually take "int" parameters.
FUSE unfortunately assumed that the ioctl definitions never lie, and
transfers a long's worth of data in and out of userspace, which causes
stack smashing in chattr, and other bugs elsewhere.

So, special-case this in FUSE so that we don't crash userland.

v2: Do the same for the IOC_[GS]ETVERSION ioctls, as Richard Hansen
    points out.

Signed-off-by: Darrick J. Wong <darrick.wong-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
 fs/fuse/file.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 7e70506..f8766ab 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2385,6 +2385,22 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg,
 		iov->iov_base = (void __user *)arg;
 		iov->iov_len = _IOC_SIZE(cmd);
 
+		/*
+		 * The IOC_[GS]ETFLAGS and IOC_[GS]ETVERSION ioctls take int
+		 * parameters even though the ioctl definition specifies long.
+		 * Userland has been expecting int for ages (and chattr
+		 * segfaults on FUSE filesystems), so special case that here.
+		 * The IOC32 variants were declared with int, so they don't
+		 * need this correction.
+		 */
+		switch (cmd) {
+		case FS_IOC_GETFLAGS:
+		case FS_IOC_SETFLAGS:
+		case FS_IOC_GETVERSION:
+		case FS_IOC_SETVERSION:
+			iov->iov_len = sizeof(int);
+		}
+
 		if (_IOC_DIR(cmd) & _IOC_WRITE) {
 			in_iov = iov;
 			in_iovs = 1;

------------------------------------------------------------------------------
Rapidly troubleshoot problems before they affect your business. Most IT 
organizations don't have a clear picture of how application performance 
affects their revenue. With AppDynamics, you get 100% visibility into your 
Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro!
http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk

Re: [PATCH v2] fuse: Fix IOC_[GS]ET{FLAGS,VERSION} argument size brokenness.

[Andreas Dilger]

To be honest, FS_IOC_SETVERSION isn't used by many/any users, so it might be better to avoid doing anything with that for now.

In the past we even talked about adding a deprecation warning for that ioctl since it adds complexity for little value. 

Cheers, Andreas

> On Dec 20, 2013, at 16:35, "Darrick J. Wong" <darrick.wong@oracle.com> wrote:
> 
> The IOC_[GS]ETFLAGS and IOC_[GS]ETVERSION ioctls, despite being
> defined to take a "long" parameter, actually take "int" parameters.
> FUSE unfortunately assumed that the ioctl definitions never lie, and
> transfers a long's worth of data in and out of userspace, which causes
> stack smashing in chattr, and other bugs elsewhere.
> 
> So, special-case this in FUSE so that we don't crash userland.
> 
> v2: Do the same for the IOC_[GS]ETVERSION ioctls, as Richard Hansen
>    points out.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> fs/fuse/file.c |   16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 7e70506..f8766ab 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -2385,6 +2385,22 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg,
>        iov->iov_base = (void __user *)arg;
>        iov->iov_len = _IOC_SIZE(cmd);
> 
> +        /*
> +         * The IOC_[GS]ETFLAGS and IOC_[GS]ETVERSION ioctls take int
> +         * parameters even though the ioctl definition specifies long.
> +         * Userland has been expecting int for ages (and chattr
> +         * segfaults on FUSE filesystems), so special case that here.
> +         * The IOC32 variants were declared with int, so they don't
> +         * need this correction.
> +         */
> +        switch (cmd) {
> +        case FS_IOC_GETFLAGS:
> +        case FS_IOC_SETFLAGS:
> +        case FS_IOC_GETVERSION:
> +        case FS_IOC_SETVERSION:
> +            iov->iov_len = sizeof(int);
> +        }
> +
>        if (_IOC_DIR(cmd) & _IOC_WRITE) {
>            in_iov = iov;
>            in_iovs = 1;
> --
> 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: [PATCH v2] fuse: Fix IOC_[GS]ET{FLAGS,VERSION} argument size brokenness.

[Darrick J. Wong]

On Fri, Dec 20, 2013 at 07:09:23PM -0700, Andreas Dilger wrote:
> To be honest, FS_IOC_SETVERSION isn't used by many/any users, so it might be
> better to avoid doing anything with that for now.
> 
> In the past we even talked about adding a deprecation warning for that ioctl
> since it adds complexity for little value. 

I suspect that most FUSE servers don't handle any of these four ioctls,
but so long as they're there, we ought to make them less surprising to
userspace.

As far as getting rid of SETVERSION, I think that's best left to a fs driver
(or a fuse server) to make that decision, unless everyone wants to eliminate
SETVERSION entirely?  I don't remember anyone complaining when I proposed to
disable SETVERSION on metadata_csum ext4 filesystems, but that's hardly
conclusive. :)

The only providers of SETVERSION seem to be ext[234], reiser3, and ocfs2.

--D

> 
> Cheers, Andreas
> 
> > On Dec 20, 2013, at 16:35, "Darrick J. Wong" <darrick.wong@oracle.com> wrote:
> > 
> > The IOC_[GS]ETFLAGS and IOC_[GS]ETVERSION ioctls, despite being
> > defined to take a "long" parameter, actually take "int" parameters.
> > FUSE unfortunately assumed that the ioctl definitions never lie, and
> > transfers a long's worth of data in and out of userspace, which causes
> > stack smashing in chattr, and other bugs elsewhere.
> > 
> > So, special-case this in FUSE so that we don't crash userland.
> > 
> > v2: Do the same for the IOC_[GS]ETVERSION ioctls, as Richard Hansen
> >    points out.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > fs/fuse/file.c |   16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> > 
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index 7e70506..f8766ab 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -2385,6 +2385,22 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg,
> >        iov->iov_base = (void __user *)arg;
> >        iov->iov_len = _IOC_SIZE(cmd);
> > 
> > +        /*
> > +         * The IOC_[GS]ETFLAGS and IOC_[GS]ETVERSION ioctls take int
> > +         * parameters even though the ioctl definition specifies long.
> > +         * Userland has been expecting int for ages (and chattr
> > +         * segfaults on FUSE filesystems), so special case that here.
> > +         * The IOC32 variants were declared with int, so they don't
> > +         * need this correction.
> > +         */
> > +        switch (cmd) {
> > +        case FS_IOC_GETFLAGS:
> > +        case FS_IOC_SETFLAGS:
> > +        case FS_IOC_GETVERSION:
> > +        case FS_IOC_SETVERSION:
> > +            iov->iov_len = sizeof(int);
> > +        }
> > +
> >        if (_IOC_DIR(cmd) & _IOC_WRITE) {
> >            in_iov = iov;
> >            in_iovs = 1;
> > --
> > 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: [fuse-devel] [PATCH v2] fuse: Fix IOC_[GS]ET{FLAGS, VERSION} argument size brokenness.

[Miklos Szeredi]

On Fri, Dec 20, 2013 at 03:35:34PM -0800, Darrick J. Wong wrote:
> The IOC_[GS]ETFLAGS and IOC_[GS]ETVERSION ioctls, despite being
> defined to take a "long" parameter, actually take "int" parameters.
> FUSE unfortunately assumed that the ioctl definitions never lie, and

FUSE doesn't really assume anything.  It just says, that ioctl's that *properly*
use the _IO* macros will work.  Everything else will not.

It's unfortunate that IOC_SET/GETFLAGS is something that may actually make sense
on a fuse filesystem...

Two ways out of this:

 A) Add quirks to fuse
 B) Add new, fixed ioctls and use those from chattr if available

I'd very much prefer B.

Thanks,
Miklos

> transfers a long's worth of data in and out of userspace, which causes
> stack smashing in chattr, and other bugs elsewhere.
> 
> So, special-case this in FUSE so that we don't crash userland.
> 
> v2: Do the same for the IOC_[GS]ETVERSION ioctls, as Richard Hansen
>     points out.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/fuse/file.c |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 7e70506..f8766ab 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -2385,6 +2385,22 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg,
>  		iov->iov_base = (void __user *)arg;
>  		iov->iov_len = _IOC_SIZE(cmd);
>  
> +		/*
> +		 * The IOC_[GS]ETFLAGS and IOC_[GS]ETVERSION ioctls take int
> +		 * parameters even though the ioctl definition specifies long.
> +		 * Userland has been expecting int for ages (and chattr
> +		 * segfaults on FUSE filesystems), so special case that here.
> +		 * The IOC32 variants were declared with int, so they don't
> +		 * need this correction.
> +		 */
> +		switch (cmd) {
> +		case FS_IOC_GETFLAGS:
> +		case FS_IOC_SETFLAGS:
> +		case FS_IOC_GETVERSION:
> +		case FS_IOC_SETVERSION:
> +			iov->iov_len = sizeof(int);
> +		}
> +
>  		if (_IOC_DIR(cmd) & _IOC_WRITE) {
>  			in_iov = iov;
>  			in_iovs = 1;
> 
> ------------------------------------------------------------------------------
> Rapidly troubleshoot problems before they affect your business. Most IT 
> organizations don't have a clear picture of how application performance 
> affects their revenue. With AppDynamics, you get 100% visibility into your 
> Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro!
> http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk
> _______________________________________________
> fuse-devel mailing list
> fuse-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/fuse-devel

Re: [fuse-devel] [PATCH v2] fuse: Fix IOC_[GS]ET{FLAGS, VERSION} argument size brokenness.

[Darrick J. Wong]

On Mon, Jan 06, 2014 at 06:50:20PM +0100, Miklos Szeredi wrote:
> On Fri, Dec 20, 2013 at 03:35:34PM -0800, Darrick J. Wong wrote:
> > The IOC_[GS]ETFLAGS and IOC_[GS]ETVERSION ioctls, despite being
> > defined to take a "long" parameter, actually take "int" parameters.
> > FUSE unfortunately assumed that the ioctl definitions never lie, and
> 
> FUSE doesn't really assume anything.  It just says, that ioctl's that *properly*
> use the _IO* macros will work.  Everything else will not.
> 
> It's unfortunate that IOC_SET/GETFLAGS is something that may actually make sense
> on a fuse filesystem...
> 
> Two ways out of this:
> 
>  A) Add quirks to fuse
>  B) Add new, fixed ioctls and use those from chattr if available
> 
> I'd very much prefer B.

Ok.  I'll work on a new interface, then.  It was suggested that I explore an
interface that isn't limited to bitmasks.  The xattr interface seems like it
could be hooked for that, so I'll play with that for a while.

Something along the lines of:

attr -s system.iflags -V e,i,c,weird_fs_specific_flag /path/to/file ?

(Pretend that attr will actually set system.* attributes, which it won't.)

--D
> 
> Thanks,
> Miklos
> 
> > transfers a long's worth of data in and out of userspace, which causes
> > stack smashing in chattr, and other bugs elsewhere.
> > 
> > So, special-case this in FUSE so that we don't crash userland.
> > 
> > v2: Do the same for the IOC_[GS]ETVERSION ioctls, as Richard Hansen
> >     points out.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/fuse/file.c |   16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index 7e70506..f8766ab 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -2385,6 +2385,22 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg,
> >  		iov->iov_base = (void __user *)arg;
> >  		iov->iov_len = _IOC_SIZE(cmd);
> >  
> > +		/*
> > +		 * The IOC_[GS]ETFLAGS and IOC_[GS]ETVERSION ioctls take int
> > +		 * parameters even though the ioctl definition specifies long.
> > +		 * Userland has been expecting int for ages (and chattr
> > +		 * segfaults on FUSE filesystems), so special case that here.
> > +		 * The IOC32 variants were declared with int, so they don't
> > +		 * need this correction.
> > +		 */
> > +		switch (cmd) {
> > +		case FS_IOC_GETFLAGS:
> > +		case FS_IOC_SETFLAGS:
> > +		case FS_IOC_GETVERSION:
> > +		case FS_IOC_SETVERSION:
> > +			iov->iov_len = sizeof(int);
> > +		}
> > +
> >  		if (_IOC_DIR(cmd) & _IOC_WRITE) {
> >  			in_iov = iov;
> >  			in_iovs = 1;
> > 
> > ------------------------------------------------------------------------------
> > Rapidly troubleshoot problems before they affect your business. Most IT 
> > organizations don't have a clear picture of how application performance 
> > affects their revenue. With AppDynamics, you get 100% visibility into your 
> > Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro!
> > http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk
> > _______________________________________________
> > fuse-devel mailing list
> > fuse-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/fuse-devel