[2/3] xfs: disallow broken ioctls without compat-32-bit-time

Message ID 20191218163954.296726-2-arnd@arndb.de
State New
Headers show
Series
  • [1/3] xfs: rename compat_time_t to old_time32_t
Related show

Commit Message

Arnd Bergmann Dec. 18, 2019, 4:39 p.m.
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 once the extended times are supported.

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

Christoph Hellwig Dec. 24, 2019, 8:45 a.m. | #1
On Wed, Dec 18, 2019 at 05:39:29PM +0100, Arnd Bergmann wrote:
> +/* 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;


I think the check for the individual command belongs into the callers,
which laves us with:

static inline bool have_time32(void)
{
	return IS_ENABLED(CONFIG_COMPAT_32BIT_TIME) ||
		(IS_ENABLED(CONFIG_64BIT) && !in_compat_syscall());
}

and that looks like it should be in a generic helper somewhere.


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


Here we can simply check for cmd != XFS_IOC_FSINUMBERS before the call.

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

> +	}


And for this one we just have one cmd anyway.  But I actually still
disagree with the old_time check for this one entirely, as voiced on
one of the last iterations.  For swapext the time stamp really is
only used as a generation counter, so overflows are entirely harmless.

Patch

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 7b35d62ede9f..d43582e933a0 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) {