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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200102180749.GA1508633@magnolia>
Date:   Thu, 2 Jan 2020 10:07:49 -0800
From:   "Darrick J. Wong" <darrick.wong@...cle.com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Christoph Hellwig <hch@...radead.org>,
        linux-xfs <linux-xfs@...r.kernel.org>,
        y2038 Mailman List <y2038@...ts.linaro.org>,
        Brian Foster <bfoster@...hat.com>,
        Dave Chinner <dchinner@...hat.com>,
        Allison Collins <allison.henderson@...cle.com>,
        Jan Kara <jack@...e.cz>, Eric Sandeen <sandeen@...deen.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] xfs: disallow broken ioctls without
 compat-32-bit-time

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