On Wed, Oct 31, 2018 at 4:49 AM Ian Abbott <abbotti@xxxxxxxxx> wrote:
>
> On 25/10/18 02:36, Spencer E. Olson 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>
> > ---
> > drivers/staging/comedi/comedi_fops.c | 37 +++++++++++++++++-----------
> > 1 file changed, 22 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
> > index c1c6b2b4ab91..a163ec2df872 100644
> > --- a/drivers/staging/comedi/comedi_fops.c
> > +++ b/drivers/staging/comedi/comedi_fops.c
> > @@ -1500,7 +1500,7 @@ 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 MAX_SAMPLES 65536
> > static int do_insnlist_ioctl(struct comedi_device *dev,
> > struct comedi_insnlist __user *arg, void *file)
> > {
> > @@ -1513,12 +1513,6 @@ static int do_insnlist_ioctl(struct comedi_device *dev,
> > 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;
> > @@ -1539,6 +1533,14 @@ static int do_insnlist_ioctl(struct comedi_device *dev,
> > ret = -EINVAL;
> > goto error;
> > }
> > +
> > + data = kmalloc_array(insns[i].n, sizeof(unsigned int),
> > + GFP_KERNEL);
> > + if (!data) {
> > + ret = -ENOMEM;
> > + goto error;
> > + }
> > +
>
> Something you could do here is work out which insn->n in the instruction
> list is the largest, and allocate the 'data' once outside the
> instruction list handling loop instead of allocating it inside the loop.
I just realized that I left this unfinished. How much headway did you
already make with fixing the various other drivers that had previously
not checked length of insn->n?
(I'll try your last suggestion and get that submitted in the next day or so.)
>
> --
> -=( Ian Abbott <abbotti@xxxxxxxxx> || Web: www.mev.co.uk )=-
> -=( MEV Ltd. is a company registered in England & Wales. )=-
> -=( Registered number: 02862268. Registered address: )=-
> -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
