| 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
| ||
|
Message-ID: <SN6PR11MB2574F33CF9C422517B3C13CFF67C0@SN6PR11MB2574.namprd11.prod.outlook.com>
Date: Fri, 17 Jul 2020 18:19:52 +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
> > +/* Verify the ioctl argument size and copy the argument into kernel
> > +memory */ static int dlb2_copy_from_user(struct dlb2_dev *dev,
> > + unsigned long user_arg,
> > + u16 user_size,
> > + void *arg,
> > + size_t size) {
> > + if (user_size != size) {
> > + dev_err(dev->dlb2_device,
> > + "[%s()] Invalid ioctl size\n", __func__);
> > + return -EINVAL;
> > + }
> > +
> > + if (copy_from_user(arg, (void __user *)user_arg, size)) {
> > + dev_err(dev->dlb2_device,
> > + "[%s()] Invalid ioctl argument pointer\n", __func__);
> > + return -EFAULT;
> > + }
> > +
> > + return 0;
> > +}
>
> You should avoid error messages that are triggered based on user input.
> and can cause a denial-of-service when the console is spammed that way.
>
Makes sense, will fix.
> 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.
>
> > +/* [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.
>
> > +static int dlb2_ioctl_create_sched_domain(struct dlb2_dev *dev,
> > + unsigned long user_arg,
> > + u16 size) {
> > + struct dlb2_create_sched_domain_args arg;
> > + struct dlb2_cmd_response response = {0};
> > + int ret;
> > +
> > + dev_dbg(dev->dlb2_device, "Entering %s()\n", __func__);
>
> I assume you have finished debugging this version and can remove these
> debug comments again. If you find new bugs, you can add them temporarily,
> but nothing is gained by these here. You can use ftrace to see when functions
> are called.
You're right, and this was mentioned in another comment as well. I'll remove these unnecessary dev_dbg() calls.
>
> > +
> > + ret = dlb2_copy_from_user(dev, user_arg, size, &arg, sizeof(arg));
> > + if (ret)
> > + return ret;
> > +
> > + /* Copy zeroes to verify the user-provided response pointer */
> > + ret = dlb2_copy_resp_to_user(dev, arg.response, &response);
> > + if (ret)
> > + return ret;
>
> There is no need to verify the response pointer. If it fails later, it's the user's
> fault, and they get to deal with it.
>
> > +static int dlb2_ioctl_get_driver_version(struct dlb2_dev *dev,
> > + unsigned long user_arg,
> > + u16 size) {
> > + struct dlb2_get_driver_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_VERSION;
>
> Just remove the driver version command, trying to have explicit interface
> versions creates more problems than it solves.
(See question above)
>
> > +int dlb2_ioctl_dispatcher(struct dlb2_dev *dev,
> > + unsigned int cmd,
> > + unsigned long arg) {
> > + u16 sz = _IOC_SIZE(cmd);
> > +
> > + if (_IOC_NR(cmd) >= NUM_DLB2_CMD) {
> > + dev_err(dev->dlb2_device,
> > + "[%s()] Unexpected DLB command %d\n",
> > + __func__, _IOC_NR(cmd));
> > + return -1;
> > + }
> > +
> > + return dlb2_ioctl_callback_fns[_IOC_NR(cmd)](dev, arg, sz); }
>
> This is usually written with a switch/case statement, so doing the same here
> tends to make it easier to understand.
>
Will do. And as Randy noticed, this needed array_index_nospec() -- easier to avoid that entirely with a switch statement.
> > +static long
> > +dlb2_ioctl(struct file *f, unsigned int cmd, unsigned long arg) {
> > + struct dlb2_dev *dev;
> > +
> > + dev = container_of(f->f_inode->i_cdev, struct dlb2_dev, cdev);
> > +
> > + if (_IOC_TYPE(cmd) != DLB2_IOC_MAGIC) {
> > + dev_err(dev->dlb2_device,
> > + "[%s()] Bad magic number!\n", __func__);
> > + return -EINVAL;
> > + }
> > +
> > + return dlb2_ioctl_dispatcher(dev, cmd, arg); }
>
> 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.
Thanks,
Gage
>
> Arnd
Powered by blists - more mailing lists