Re: [PATCH] staging: comedi: change do_insn*_ioctl to allow more samples

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Oct 30, 2018 at 6:21 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;
> > +             }
> > +
> >               if (insns[i].insn & INSN_MASK_WRITE) {
> >                       if (copy_from_user(data, insns[i].data,
> >                                          insns[i].n * sizeof(unsigned int))) {
> > @@ -1560,6 +1562,11 @@ static int do_insnlist_ioctl(struct comedi_device *dev,
> >                               goto error;
> >                       }
> >               }
> > +
> > +             /* free up the single-instruction data memory */
> > +             kfree(data);
> > +             data = NULL;
> > +
> >               if (need_resched())
> >                       schedule();
> >       }
> > @@ -1594,20 +1601,20 @@ static int do_insn_ioctl(struct comedi_device *dev,
> >       unsigned int *data = NULL;
> >       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;
> >       }
> >
> >       /* This is where the behavior of insn and insnlist deviate. */
> >       if (insn.n > MAX_SAMPLES)
> >               insn.n = MAX_SAMPLES;
> > +
> > +     data = kmalloc_array(insn.n, sizeof(unsigned int), GFP_KERNEL);
> > +     if (!data) {
> > +             ret = -ENOMEM;
> > +             goto error;
> > +     }
> > +
> >       if (insn.insn & INSN_MASK_WRITE) {
> >               if (copy_from_user(data,
> >                                  insn.data,
> >
>
> There are still some broken drivers that do not check insn->n and assume
> the data has some minimum length, so we will need to make the data array
> some minimum length (say 16 * sizeof(unsigned int)) until those drivers
> have been fixed.
>

I don't think that would be much of a problem, since 16*sizeof(uint)
is pretty negligible.  How certain are you that we would not have to
make that higher, as the previous default was a length of
256*sizeof(uint)?  It'll take me a day or two to resubmit (have other
things pressing on my time).

> For example (there might be other places) ...
>
> In cb_pcidas64.c:
>
> * ao_winsn() (the insn_write handler for the AO subdevice) assumes that
> insn->n is 1 and reads data[0], but insn->n could be 0.
> * eeprom_read_insn() (the insn_read handler for the serial EEPROM) also
> assumes insn->n is 1 and accesses data[0].
> * ai_config_master_clock() (the handler for INSN_CONFIG_TIMER_1) assumes
> insn->n is at least 5 but nothing checks it.  (It isn't checked by
> check_insn_config_length() in comedi_fops.c currently.)
>
> In cb_pcidda.c:
>
> * cb_pcidda_ao_insn_write() assumes insn->n is 1 and that data[0] is valid.
>
> In ni_labpc_common.c:
>
> * labpc_ao_insn_write() assumes insn->n is 1 and that data[0] is valid.
>
> In ni_mio_common.c:
>
> * ni_calib_insn_read(), ni_eeprom_insn_read(),
> ni_m_series_eeprom_insn_read(), ni_calib_insn_write() all assume insn->n
> is 1 and that data[0] is valid or accessible.
>
> In s526.c:
>
> * s526_gpct_winsn() uses data[0] and sometimes data[1] but doesn't check
> insn->n.
> * s526_gpct_insn_config() accesses up to data[3] but doesn't check the
> length (and check_insn_config_length() in comedi_fops.c doesn't check
> the lengths for the INSN_CONFIG instructions handled here).
>
> --
> -=( 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



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux