On Tue, Dec 4, 2018 at 12:08 PM Spencer E. Olson <olsonse@xxxxxxxxx> 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 <olsonse@xxxxxxxxx> > --- > 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 devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel