linux-kernel - Re: [PATCH v5 01/18] xfs: compat_ioctl: use compat

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 for Android: free password hash cracker in your pocket

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

Date:   Thu, 15 Aug 2019 09:56:54 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     Dave Chinner <david@...morbit.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Linux FS-devel Mailing List <linux-fsdevel@...r.kernel.org>,
        "Darrick J. Wong" <darrick.wong@...cle.com>,
        linux-xfs <linux-xfs@...r.kernel.org>,
        Brian Foster <bfoster@...hat.com>,
        Allison Collins <allison.henderson@...cle.com>,
        Nick Bowler <nbowler@...conx.ca>,
        Eric Sandeen <sandeen@...deen.net>,
        Dave Chinner <dchinner@...hat.com>
Subject: Re: [PATCH v5 01/18] xfs: compat_ioctl: use compat_ptr()

On Thu, Aug 15, 2019 at 9:13 AM Christoph Hellwig <hch@...radead.org> wrote:
>
> On Thu, Aug 15, 2019 at 07:37:53AM +1000, Dave Chinner wrote:
> > > @@ -576,7 +576,7 @@ xfs_file_compat_ioctl(
> > >     case XFS_IOC_SCRUB_METADATA:
> > >     case XFS_IOC_BULKSTAT:
> > >     case XFS_IOC_INUMBERS:
> > > -           return xfs_file_ioctl(filp, cmd, p);
> > > +           return xfs_file_ioctl(filp, cmd, (unsigned long)arg);
> >
> > I don't really like having to sprinkle special casts through the
> > code because of this.
>
> True.  But the proper fix is to not do the indirection through
> xfs_file_ioctl but instead to call xfs_ioc_scrub_metadata,
> xfs_ioc_bulkstat, etc directly which all take a void __user
> arguments already.

I'm not sure that's better: This would end up duplicating all
of xfs_file_ioctl(), which is already a fairly long function, compared
to the current way of having a large set of commands all handled
with a single line.

>From looking at other subsystems, what I find to work best is to
move the compat handler into the same file as the native code
and then structure the files so that shared handlers get
put into one place, something like

/* these are the ones that have the same ABI for 32-bit and 64-bit tasks */
static int xfs_compatible_file_ioctl(struct file *filp, unsigned cmd,
void __user *p)
{
      int ret = -ENOIOCTLCMD;

       switch (cmd) {
       case XFS_IOC_DIOINFO:
            ...
        case ...
     }

     return ret;
}

long
xfs_file_compat_ioctl(
        struct file             *filp,
        unsigned                cmd,
        unsigned long           p)
{
       ret = xfs_compatible_file_ioctl(filp, cmd, compat_ptr(p));
       if (ret != -ENOIOCTLCMD)
              return ret;

      /* all incompatible ones below */
      switch (cmd) {
         ...
      }
}
Having them in one place makes it more obvious to readers how the
native and compat handlers fit together, and makes it easier to keep
the two in sync.

That would of course be a much larger change to how it's done today,
and it's way out of scope of what I want to achieve in my (already
too long) series.

     Arnd