linux-kernel - RE: [PATCH 04/20] dlb2: add device ioctl layer and first 4 ioctls

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]

Date:   Fri, 17 Jul 2020 20:05:08 +0000
From:   "Eads, Gage" <gage.eads@...el.com>
To:     Arnd Bergmann <arnd@...db.de>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        gregkh <gregkh@...uxfoundation.org>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        "Topel, Bjorn" <bjorn.topel@...el.com>
Subject: RE: [PATCH 04/20] dlb2: add device ioctl layer and first 4 ioctls



> -----Original Message-----
> From: Arnd Bergmann <arnd@...db.de>
> Sent: Friday, July 17, 2020 1:57 PM
> To: Eads, Gage <gage.eads@...el.com>
> Cc: linux-kernel@...r.kernel.org; gregkh <gregkh@...uxfoundation.org>;
> Karlsson, Magnus <magnus.karlsson@...el.com>; Topel, Bjorn
> <bjorn.topel@...el.com>
> Subject: Re: [PATCH 04/20] dlb2: add device ioctl layer and first 4 ioctls
> 
> On Fri, Jul 17, 2020 at 8:19 PM Eads, Gage <gage.eads@...el.com> wrote:
> 
> > > A plain copy_from_user() in place of this function should be fine.
> >
> > This function also validates the user size arg to prevent buffer overflow;
> centralizing it here avoids the case where a programmer accidentally forgets
> the check in an ioctl handler (and reduces code duplication). If it's alright with
> you, I'll keep the function but drop the dev_err() prints.
> 
> Once you use a 'switch(cmd)' statement in the top ioctl handler, the data
> structure size will be fixed, so there is no way the argument size can go wrong.
> 

Ah, understood. Will fix in v2.

> > >
> > > > +/* [7:0]: device revision, [15:8]: device version */ #define
> > > > +DLB2_SET_DEVICE_VERSION(ver, rev) (((ver) << 8) | (rev))
> > > > +
> > > > +static int dlb2_ioctl_get_device_version(struct dlb2_dev *dev,
> > > > +                                        unsigned long user_arg,
> > > > +                                        u16 size) {
> > > > +       struct dlb2_get_device_version_args arg;
> > > > +       struct dlb2_cmd_response response;
> > > > +       int ret;
> > > > +
> > > > +       dev_dbg(dev->dlb2_device, "Entering %s()\n", __func__);
> > > > +
> > > > +       response.status = 0;
> > > > +       response.id = DLB2_SET_DEVICE_VERSION(2, DLB2_REV_A0);
> > > > +
> > > > +       ret = dlb2_copy_from_user(dev, user_arg, size, &arg, sizeof(arg));
> > > > +       if (ret)
> > > > +               return ret;
> > > > +
> > > > +       ret = dlb2_copy_resp_to_user(dev, arg.response,
> > > > + &response);
> > >
> > > Better avoid any indirect pointers. As you always return a constant
> > > here, I think the entire ioctl command can be removed until you
> > > actually need it. If you have an ioctl command that needs both input
> > > and output, use _IOWR() to define it and put all arguments into the same
> structure.
> >
> > Ok, I'll merge the response structure into the ioctl structure (here and
> elsewhere).
> >
> > Say I add this command later: without driver versioning, how would
> > user-space know in advance whether the command is supported?
> > It could attempt the command and interpret -ENOTTY as "unsupported",
> > but that strikes me as an inelegant way to reverse-engineer the version.
> 
> There is not really a driver "version" once the driver is upstream, the concept
> doesn't really make sense here when arbitrary patches can get backported
> from the latest kernel into whatever the user is running.
> 

"Driver interface version" is the better term for what I'm trying to accomplish here. Any backports would have to be done in such a way that the interface version is honored, but if that can't be reasonably expected...then I agree, versioning is unworkable.

> The ENOTTY check is indeed the normal way that user space deals with
> interfaces that may have been added later. What you normally want is to
> keep using the original interfaces anyway, unless you absolutely need a later
> revision for a certain feature, and in that case the user space program will fail
> no matter what.
> 
> > > This function can also be removed then, just call the dispatcher directly.
> > > >         int err;
> > > >
> > > > -       pr_info("%s\n", dlb2_driver_name);
> > > > +       pr_info("%s - version %d.%d.%d\n", dlb2_driver_name,
> > > > +               DLB2_VERSION_MAJOR_NUMBER,
> > > > +               DLB2_VERSION_MINOR_NUMBER,
> > > > +               DLB2_VERSION_REVISION_NUMBER);
> > > >         pr_info("%s\n", dlb2_driver_copyright);
> > >
> > > Just remove the pr_info completely.
> >
> > Can you elaborate? Printing the driver name/copyright/etc. seems to be a
> common pattern in upstream drivers.
> 
> Most drivers don't do it, and it's generally not recommended. You can print a
> message when something goes wrong, but most users don't care about that
> stuff and it clutters up the kernel log if each driver prints a line or two.
> 

Fair enough. I'll remove it.

Thanks,
Gage

>      Arnd