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

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Jan 02, 2020 at 10:16:21AM +0100, Arnd Bergmann wrote:
> On Tue, Dec 24, 2019 at 9:45 AM Christoph Hellwig <hch@infradead.org> 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.

I prefer we replace the old SWAPEXT with a new version to get rid of
struct xfs_bstat.  Though a SWAPEXT_V5 probably only needs to contain
the *stat fields that swapext actually needs to check that the file
hasn't been changed, which would be ino/gen/btime/ctime.

(Maybe I'd add an offset/length too...)

--D

>        Arnd