diff mbox series

[v2,20/24] xfs: disallow broken ioctls without compat-32-bit-time

Message ID 20191213205417.3871055-11-arnd@arndb.de
State Superseded
Headers show
Series drivers, fs: y2038 updates | expand

Commit Message

Arnd Bergmann Dec. 13, 2019, 8:53 p.m. UTC 
When building a kernel that disables support for 32-bit time_t
system calls, it also makes sense to disable the old xfs_bstat
ioctls completely, as they truncate the timestamps to 32-bit
values.

Any application using these needs to be updated to use the v5
interfaces.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 fs/xfs/xfs_ioctl.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

-- 
2.20.0

Comments

Darrick J. Wong Dec. 13, 2019, 9:05 p.m. UTC | #1 
On Fri, Dec 13, 2019 at 09:53:48PM +0100, Arnd Bergmann wrote:
> When building a kernel that disables support for 32-bit time_t

> system calls, it also makes sense to disable the old xfs_bstat

> ioctls completely, as they truncate the timestamps to 32-bit

> values.


Note that current xfs doesn't support > 32-bit timestamps at all, so for
now the old bulkstat/swapext ioctls will never overflow.

Granted, I melded everyone's suggestions into a more fully formed
'bigtime' feature patchset that I'll dump out soon as part of my usual
end of year carpetbombing of the mailing list, so we likely still need
most of this patch anyway...

> Any application using these needs to be updated to use the v5

> interfaces.

> 

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> ---

>  fs/xfs/xfs_ioctl.c | 26 ++++++++++++++++++++++++++

>  1 file changed, 26 insertions(+)

> 

> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c

> index 7b35d62ede9f..a4a4eed8879c 100644

> --- a/fs/xfs/xfs_ioctl.c

> +++ b/fs/xfs/xfs_ioctl.c

> @@ -36,6 +36,7 @@

>  #include "xfs_reflink.h"

>  #include "xfs_ioctl.h"

>  

> +#include <linux/compat.h>

>  #include <linux/mount.h>

>  #include <linux/namei.h>

>  

> @@ -617,6 +618,23 @@ xfs_fsinumbers_fmt(

>  	return xfs_ibulk_advance(breq, sizeof(struct xfs_inogrp));

>  }

>  

> +/* disallow y2038-unsafe ioctls with CONFIG_COMPAT_32BIT_TIME=n */

> +static bool xfs_have_compat_bstat_time32(unsigned int cmd)


The v5 bulkstat ioctls follow an entirely separate path through
xfs_ioctl.c, so I think you don't need the @cmd parameter.

> +{

> +	if (IS_ENABLED(CONFIG_COMPAT_32BIT_TIME))

> +		return true;

> +

> +	if (IS_ENABLED(CONFIG_64BIT) && !in_compat_syscall())

> +		return true;

> +

> +	if (cmd == XFS_IOC_FSBULKSTAT_SINGLE ||

> +	    cmd == XFS_IOC_FSBULKSTAT ||

> +	    cmd == XFS_IOC_SWAPEXT)

> +		return false;

> +

> +	return true;

> +}

> +

>  STATIC int

>  xfs_ioc_fsbulkstat(

>  	xfs_mount_t		*mp,

> @@ -637,6 +655,9 @@ xfs_ioc_fsbulkstat(

>  	if (!capable(CAP_SYS_ADMIN))

>  		return -EPERM;

>  

> +	if (!xfs_have_compat_bstat_time32(cmd))

> +		return -EINVAL;

> +

>  	if (XFS_FORCED_SHUTDOWN(mp))

>  		return -EIO;

>  

> @@ -1815,6 +1836,11 @@ xfs_ioc_swapext(

>  	struct fd	f, tmp;

>  	int		error = 0;

>  

> +	if (xfs_have_compat_bstat_time32(XFS_IOC_SWAPEXT)) {


if (!xfs_have...()) ?

--D

> +		error = -EINVAL;

> +		goto out;

> +	}

> +

>  	/* Pull information for the target fd */

>  	f = fdget((int)sxp->sx_fdtarget);

>  	if (!f.file) {

> -- 

> 2.20.0

>
Arnd Bergmann Dec. 16, 2019, 4:45 p.m. UTC | #2 
On Fri, Dec 13, 2019 at 10:05 PM Darrick J. Wong
<darrick.wong@oracle.com> wrote:
>

> On Fri, Dec 13, 2019 at 09:53:48PM +0100, Arnd Bergmann wrote:

> > When building a kernel that disables support for 32-bit time_t

> > system calls, it also makes sense to disable the old xfs_bstat

> > ioctls completely, as they truncate the timestamps to 32-bit

> > values.

>

> Note that current xfs doesn't support > 32-bit timestamps at all, so for

> now the old bulkstat/swapext ioctls will never overflow.


Right, this patch originally came after my version of the 40-bit
timestamps that I dropped from the series now.

I've added "... once the extended times are supported." above now.

> Granted, I melded everyone's suggestions into a more fully formed

> 'bigtime' feature patchset that I'll dump out soon as part of my usual

> end of year carpetbombing of the mailing list, so we likely still need

> most of this patch anyway...


What is the timeline for that work now? I'm mainly interested in
getting the removal of 'time_t/timeval/timespec' and 'get_seconds()'
from the kernel done for v5.6, but it would be good to also have
this patch and the extended timestamps in the same version
just so we can claim that "all known y2038 issues" are addressed
in that release (I'm sure we will run into bugs we don't know yet).

> > @@ -617,6 +618,23 @@ xfs_fsinumbers_fmt(

> >       return xfs_ibulk_advance(breq, sizeof(struct xfs_inogrp));

> >  }

> >

> > +/* disallow y2038-unsafe ioctls with CONFIG_COMPAT_32BIT_TIME=n */

> > +static bool xfs_have_compat_bstat_time32(unsigned int cmd)

>

> The v5 bulkstat ioctls follow an entirely separate path through

> xfs_ioctl.c, so I think you don't need the @cmd parameter.


The check is there to not forbid XFS_IOC_FSINUMBERS at
the moment, since that is not affected.

> > @@ -1815,6 +1836,11 @@ xfs_ioc_swapext(

> >       struct fd       f, tmp;

> >       int             error = 0;

> >

> > +     if (xfs_have_compat_bstat_time32(XFS_IOC_SWAPEXT)) {

>

> if (!xfs_have...()) ?


Right, fixed now.

       Arnd
Darrick J. Wong Dec. 16, 2019, 4:52 p.m. UTC | #3 
On Mon, Dec 16, 2019 at 05:45:29PM +0100, Arnd Bergmann wrote:
> On Fri, Dec 13, 2019 at 10:05 PM Darrick J. Wong

> <darrick.wong@oracle.com> wrote:

> >

> > On Fri, Dec 13, 2019 at 09:53:48PM +0100, Arnd Bergmann wrote:

> > > When building a kernel that disables support for 32-bit time_t

> > > system calls, it also makes sense to disable the old xfs_bstat

> > > ioctls completely, as they truncate the timestamps to 32-bit

> > > values.

> >

> > Note that current xfs doesn't support > 32-bit timestamps at all, so for

> > now the old bulkstat/swapext ioctls will never overflow.

> 

> Right, this patch originally came after my version of the 40-bit

> timestamps that I dropped from the series now.

> 

> I've added "... once the extended times are supported." above now.

> 

> > Granted, I melded everyone's suggestions into a more fully formed

> > 'bigtime' feature patchset that I'll dump out soon as part of my usual

> > end of year carpetbombing of the mailing list, so we likely still need

> > most of this patch anyway...

> 

> What is the timeline for that work now? I'm mainly interested in

> getting the removal of 'time_t/timeval/timespec' and 'get_seconds()'

> from the kernel done for v5.6, but it would be good to also have

> this patch and the extended timestamps in the same version

> just so we can claim that "all known y2038 issues" are addressed

> in that release (I'm sure we will run into bugs we don't know yet).


Personally, I think you should push this whenever it's ready.  Are you
aiming to send all 24 patches as a treewide pull request directly to
Linus, or would you rather the 2-3 xfs patches go through the xfs tree?

The y2038 format changes are going to take a while to push through
review.  If somehow it all gets through review for 5.6 I can always
apply both and fix the merge damage, but more likely y2038 timestamps is
a <cough> 5.8 EXPERIMENTAL thing.

Or later, given that Dave and I both have years worth of unreviewed
patch backlog. :(

> > > @@ -617,6 +618,23 @@ xfs_fsinumbers_fmt(

> > >       return xfs_ibulk_advance(breq, sizeof(struct xfs_inogrp));

> > >  }

> > >

> > > +/* disallow y2038-unsafe ioctls with CONFIG_COMPAT_32BIT_TIME=n */

> > > +static bool xfs_have_compat_bstat_time32(unsigned int cmd)

> >

> > The v5 bulkstat ioctls follow an entirely separate path through

> > xfs_ioctl.c, so I think you don't need the @cmd parameter.

> 

> The check is there to not forbid XFS_IOC_FSINUMBERS at

> the moment, since that is not affected.


Aha.

> > > @@ -1815,6 +1836,11 @@ xfs_ioc_swapext(

> > >       struct fd       f, tmp;

> > >       int             error = 0;

> > >

> > > +     if (xfs_have_compat_bstat_time32(XFS_IOC_SWAPEXT)) {

> >

> > if (!xfs_have...()) ?

> 

> Right, fixed now.


<nod>

--D

>        Arnd
Arnd Bergmann Dec. 17, 2019, 3:06 p.m. UTC | #4 
On Mon, Dec 16, 2019 at 5:52 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> On Mon, Dec 16, 2019 at 05:45:29PM +0100, Arnd Bergmann wrote:

> > On Fri, Dec 13, 2019 at 10:05 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:

> > What is the timeline for that work now? I'm mainly interested in

> > getting the removal of 'time_t/timeval/timespec' and 'get_seconds()'

> > from the kernel done for v5.6, but it would be good to also have

> > this patch and the extended timestamps in the same version

> > just so we can claim that "all known y2038 issues" are addressed

> > in that release (I'm sure we will run into bugs we don't know yet).

>

> Personally, I think you should push this whenever it's ready.  Are you

> aiming to send all 24 patches as a treewide pull request directly to

> Linus, or would you rather the 2-3 xfs patches go through the xfs tree?


My plan is get as much of the remaining 60 patches into maintainer
trees for v5.6 and then send a pull request for whatever remains that
has not been picked up by anyone.

The 24 patches are the ones that didn't seem worth splitting into a
separate series, aside from these I also have v4l2, alsa and nfsd
pending, plus a final cleanup that removes the then-unused
interfaces.

So if you can pick up the xfs patches, that would help me.

> The y2038 format changes are going to take a while to push through

> review.  If somehow it all gets through review for 5.6 I can always

> apply both and fix the merge damage, but more likely y2038 timestamps is

> a <cough> 5.8 EXPERIMENTAL thing.

>

> Or later, given that Dave and I both have years worth of unreviewed

> patch backlog. :(


Ok, I see.

        Arnd
diff mbox series

Patch

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 7b35d62ede9f..a4a4eed8879c 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -36,6 +36,7 @@ 
 #include "xfs_reflink.h"
 #include "xfs_ioctl.h"
 
+#include <linux/compat.h>
 #include <linux/mount.h>
 #include <linux/namei.h>
 
@@ -617,6 +618,23 @@  xfs_fsinumbers_fmt(
 	return xfs_ibulk_advance(breq, sizeof(struct xfs_inogrp));
 }
 
+/* disallow y2038-unsafe ioctls with CONFIG_COMPAT_32BIT_TIME=n */
+static bool xfs_have_compat_bstat_time32(unsigned int cmd)
+{
+	if (IS_ENABLED(CONFIG_COMPAT_32BIT_TIME))
+		return true;
+
+	if (IS_ENABLED(CONFIG_64BIT) && !in_compat_syscall())
+		return true;
+
+	if (cmd == XFS_IOC_FSBULKSTAT_SINGLE ||
+	    cmd == XFS_IOC_FSBULKSTAT ||
+	    cmd == XFS_IOC_SWAPEXT)
+		return false;
+
+	return true;
+}
+
 STATIC int
 xfs_ioc_fsbulkstat(
 	xfs_mount_t		*mp,
@@ -637,6 +655,9 @@  xfs_ioc_fsbulkstat(
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
+	if (!xfs_have_compat_bstat_time32(cmd))
+		return -EINVAL;
+
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return -EIO;
 
@@ -1815,6 +1836,11 @@  xfs_ioc_swapext(
 	struct fd	f, tmp;
 	int		error = 0;
 
+	if (xfs_have_compat_bstat_time32(XFS_IOC_SWAPEXT)) {
+		error = -EINVAL;
+		goto out;
+	}
+
 	/* Pull information for the target fd */
 	f = fdget((int)sxp->sx_fdtarget);
 	if (!f.file) {