Messages in this thread |  | | From | "Eads, Gage" <> | Subject | RE: [PATCH 04/20] dlb2: add device ioctl layer and first 4 ioctls | Date | Fri, 17 Jul 2020 18:19:52 +0000 |
| |
> > +/* 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
|  |