On Tue, Dec 4, 2018 at 12:08 PM Spencer E. Olson <[email protected]> wrote:
>
> Changes do_insn*_ioctl functions to allow for data lengths for each
> comedi_insn of up to 2^16. This patch also changes these functions to only
> allocate as much memory as is necessary for each comedi_insn, rather than
> allocating a fixed-sized scratch space.
>
> In testing some user-space code for the new INSN_DEVICE_CONFIG_GET_ROUTES
> facility with some newer hardware, I discovered that do_insn_ioctl and
> do_insnlist_ioctl limited the amount of data that can be passed into the
> kernel for insn's to a length of 256. For some newer hardware, the number
> of routes can be greater than 1000. Working around the old limits (256)
> would complicate the user-space/kernel interaction.
>
> The new upper limit is reasonable with current memory available and does
> not otherwise impact the memory footprint for any current or otherwise
> typical configuration.
>
> Signed-off-by: Spencer E. Olson <[email protected]>
> ---
> Implements Ian's suggestions to:
> 1) Provide a minimum amount of space to allocate (16*sizeof(uint)) to protect
> drivers that do not (yet) check the size of the instruction data (Ian has
> submitted several patches fixing this for other drivers already). In case
> insn.n does not get set, this minimum amount also avoids trying to allocate
> zero-length data.
> 2) Allocate the maximum required space needed for any of the instructions in
> an
> instruction list before executing the list of instructions (this, rather
> than
> allocating and freeing memory separately while iterating through the
> instruction list and executing each instruction).
>
> drivers/staging/comedi/comedi_fops.c | 48 ++++++++++++++++++----------
> 1 file changed, 31 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/comedi/comedi_fops.c
> b/drivers/staging/comedi/comedi_fops.c
> index ceb6ba5dd57c..5d2fcbfe02af 100644
> --- a/drivers/staging/comedi/comedi_fops.c
> +++ b/drivers/staging/comedi/comedi_fops.c
> @@ -1501,25 +1501,21 @@ static int parse_insn(struct comedi_device *dev,
> struct comedi_insn *insn,
> * data (for reads) to insns[].data pointers
> */
> /* arbitrary limits */
> -#define MAX_SAMPLES 256
> +#define MIN_SAMPLES 16
> +#define MAX_SAMPLES 65536
> static int do_insnlist_ioctl(struct comedi_device *dev,
> struct comedi_insnlist __user *arg, void *file)
> {
> struct comedi_insnlist insnlist;
> struct comedi_insn *insns = NULL;
> unsigned int *data = NULL;
> + unsigned int max_n_data_required = MIN_SAMPLES;
> int i = 0;
> int ret = 0;
>
> if (copy_from_user(&insnlist, arg, sizeof(insnlist)))
> return -EFAULT;
>
> - data = kmalloc_array(MAX_SAMPLES, sizeof(unsigned int), GFP_KERNEL);
> - if (!data) {
> - ret = -ENOMEM;
> - goto error;
> - }
> -
> insns = kcalloc(insnlist.n_insns, sizeof(*insns), GFP_KERNEL);
> if (!insns) {
> ret = -ENOMEM;
> @@ -1533,13 +1529,26 @@ static int do_insnlist_ioctl(struct comedi_device
> *dev,
> goto error;
> }
>
> - for (i = 0; i < insnlist.n_insns; i++) {
> + /* Determine maximum memory needed for all instructions. */
> + for (i = 0; i < insnlist.n_insns; ++i) {
> if (insns[i].n > MAX_SAMPLES) {
> dev_dbg(dev->class_dev,
> "number of samples too large\n");
> ret = -EINVAL;
> goto error;
> }
> + max_n_data_required = max(max_n_data_required, insns[i].n);
> + }
I realized just now that this patch does change behavior just bit:
the complaint, error, and exit are done _before_ any instruction is
executed, rather than after the prior instructions in the list are
executed. I argue this is actually a better behavior than partially
executing an instruction list, especially since this pre-inspection
could have already easily been done before.
> +
> + /* Allocate scratch space for all instruction data. */
> + data = kmalloc_array(max_n_data_required, sizeof(unsigned int),
> + GFP_KERNEL);
> + if (!data) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + for (i = 0; i < insnlist.n_insns; ++i) {
> if (insns[i].insn & INSN_MASK_WRITE) {
> if (copy_from_user(data, insns[i].data,
> insns[i].n * sizeof(unsigned
> int))) {
> @@ -1593,22 +1602,27 @@ static int do_insn_ioctl(struct comedi_device *dev,
> {
> struct comedi_insn insn;
> unsigned int *data = NULL;
> + unsigned int n_data = MIN_SAMPLES;
> int ret = 0;
>
> - data = kmalloc_array(MAX_SAMPLES, sizeof(unsigned int), GFP_KERNEL);
> - if (!data) {
> - ret = -ENOMEM;
> - goto error;
> - }
> -
> if (copy_from_user(&insn, arg, sizeof(insn))) {
> - ret = -EFAULT;
> - goto error;
> + return -EFAULT;
> }
>
> + n_data = max(n_data, insn.n);
> +
> /* This is where the behavior of insn and insnlist deviate. */
> - if (insn.n > MAX_SAMPLES)
> + if (insn.n > MAX_SAMPLES) {
> insn.n = MAX_SAMPLES;
> + n_data = MAX_SAMPLES;
> + }
> +
> + data = kmalloc_array(n_data, sizeof(unsigned int), GFP_KERNEL);
> + if (!data) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> if (insn.insn & INSN_MASK_WRITE) {
> if (copy_from_user(data,
> insn.data,
> --
> 2.17.1
>
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
