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

[Arnd Bergmann]

On Tue, Dec 24, 2019 at 9:45 AM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> 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.

Yes, makes sense.

I was going for something XFS specific here because XFS is unique in the
kernel in completely deprecating a set of ioctl commands (replacing
the old interface with a v5) rather than allowing the user space to be
compiled with 64-bit time_t.

If we add a global helper for this, I'd be tempted to also stick a
WARN_RATELIMIT() in there to give users a better indication of
what broke after disabling CONFIG_COMPAT_32BIT_TIME.

The same warning would make sense in the system calls, but then
we have to decide which combinations we want to allow being
configured at runtime or compile-time.

a) unmodified behavior
b) just warn but allow
c) no warning but disallow
d) warn and disallow

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

Sorry I missed that comment earlier. I've had a fresh look now, but
I think we still need to deprecate XFS_IOC_SWAPEXT and add a
v5 version of it, since the comparison will fail as soon as the range
of the inode timestamps is extended beyond 2038, otherwise the
comparison will always be false, or require comparing the truncated
time values which would add yet another representation.

       Arnd