[PATCH 02/49] staging: comedi: have core check cmd->chanlist_len min
Ian Abbott
abbotti at mev.co.uk
Wed Apr 16 15:57:29 UTC 2014
On 2014-04-15 18:37, H Hartley Sweeten wrote:
> The only time the chanlist can be NULL and chanlist_len < 1 is when a
> COMEDI_CMDTEST ioctl is done, usually for the comedilib function
> comedi_get_cmd_src_mask(). This comedilib function sets all the command
> sources to TRIG_ANY and Step 1 of the (*do_cmdtest) will fail returning
> a the mask of valid sources.
>
> For the rest of the (*do_cmdtest) and the (*do_cmd) functions a valid
> chanlist and chanlist_len >= 1 is required.
>
> Modify the check in __comedi_get_user_chanlist() to allow a NULL
> chanlist or chanlist_len < 1 only when all the command sources are
> TRIG_ANY.
>
> Remove the checks in the comedi_driver (*do_cmdtest) functions that
> don't result in to much diff noise. The remaining ones will be removed
> in following patches.
>
> Signed-off-by: H Hartley Sweeten <hsweeten at visionengravers.com>
> Cc: Ian Abbott <abbotti at mev.co.uk>
> Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
NAK. This will break existing libcomedi library function
comedi_get_generic_timed(), which will call the COMEDI_CMDTEST ioctl
with a NULL chanlist (but with chanlist_len > 1) and with start_src etc.
set to values other than TRIG_ANY.
For COMEDI_CMDTEST, chanlist is allowed to be NULL and chanlist_len is
allowed to be 0 (although some drivers' do_cmdtest handlers currently
check it is at least 1). (However, the command structure might get
modified in strange ways if the command test gets as far as "step 3"
with a zero chanlist_len, so it's best for the user-level code to set a
proper chanlist_len if it's expecting the test to get as far as "step 3".)
However, for the COMEDI_CMD ioctl, chanlist_len needs to be at least 1
and chanlist needs to be non-NULL, but its NULL-ness shouldn't be
checked until after s->do_cmdtest() has been called, as that can modify
the erroneous command before passing it back to the user.
> ---
> drivers/staging/comedi/comedi_fops.c | 31 +++++++++++++++-------
> .../comedi/drivers/addi-data/hwdrv_apci3120.c | 2 --
> drivers/staging/comedi/drivers/adl_pci9118.c | 5 +---
> drivers/staging/comedi/drivers/amplc_pci224.c | 5 ----
> drivers/staging/comedi/drivers/cb_pcidas64.c | 2 --
> drivers/staging/comedi/drivers/comedi_test.c | 1 -
> drivers/staging/comedi/drivers/das1800.c | 1 -
> drivers/staging/comedi/drivers/das800.c | 1 -
> drivers/staging/comedi/drivers/me4000.c | 16 -----------
> drivers/staging/comedi/drivers/ni_at_a2150.c | 1 -
> drivers/staging/comedi/drivers/ni_labpc.c | 9 -------
> drivers/staging/comedi/drivers/pcl812.c | 1 -
> drivers/staging/comedi/drivers/pcl816.c | 10 +++----
> drivers/staging/comedi/drivers/pcl818.c | 9 +++----
> drivers/staging/comedi/drivers/usbduxfast.c | 3 ---
> 15 files changed, 28 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
> index e283e6c..5a0e702 100644
> --- a/drivers/staging/comedi/comedi_fops.c
> +++ b/drivers/staging/comedi/comedi_fops.c
> @@ -1465,9 +1465,27 @@ static int __comedi_get_user_chanlist(struct comedi_device *dev,
> unsigned int *chanlist;
> int ret;
>
> - /* user_chanlist could be NULL for do_cmdtest ioctls */
> - if (!user_chanlist)
> - return 0;
> + /*
> + * The only time the user_chanlist is allowed to be NULL
> + * or chanlist_len < 1 is when a COMEDI_CMDTEST ioctl is
> + * done to get the mask of valid command sources. This is
> + * done by setting all the command sources to TRIG_ANY
> + * and Step 1 of the (*do_cmdtest) will fail returning a
> + * mask of valid sources.
> + */
> + if (!user_chanlist || cmd->chanlist_len < 1) {
> + if (cmd->start_src == TRIG_ANY &&
> + cmd->scan_begin_src == TRIG_ANY &&
> + cmd->convert_src == TRIG_ANY &&
> + cmd->scan_end_src == TRIG_ANY &&
> + cmd->stop_src == TRIG_ANY) {
> + return 0;
> + } else {
> + dev_err(dev->class_dev,
> + "async command must have a chanlist\n");
> + return -EINVAL;
> + }
> + }
>
> chanlist = memdup_user(user_chanlist,
> cmd->chanlist_len * sizeof(unsigned int));
I think the safest option is to leave __comedi_get_user_chanlist()
unchanged and do additional tests in do_cmd_ioctl().
Another possible change that may lead to some simplifications is to
force chanlist to be NULL if cmd->chanlist_len is 0 like this:
if (cmd->chanlist_len)
chanlist = memdup_user(user_chanlist,
cmd->chanlist_len * sizeof(unsigned int));
else
chanlist = NULL;
(Normally, the linux kernel memory allocation functions return a
non-NULL pointer value even for a zero-length allocation.)
> @@ -1518,13 +1536,6 @@ static int do_cmd_ioctl(struct comedi_device *dev,
> return -EBUSY;
> }
>
> - /* make sure channel/gain list isn't too short */
> - if (cmd.chanlist_len < 1) {
> - dev_dbg(dev->class_dev, "channel/gain list too short %u < 1\n",
> - cmd.chanlist_len);
> - return -EINVAL;
> - }
> -
Can leave that test in do_cmd_ioctl().
> async->cmd = cmd;
> async->cmd.data = NULL;
Further down do_cmd_ioctl() we have:
ret = s->do_cmdtest(dev, s, &async->cmd);
if (async->cmd.flags & TRIG_BOGUS || ret) {
dev_dbg(dev->class_dev, "test returned %d\n", ret);
cmd = async->cmd;
/* restore chanlist pointer before copying back */
cmd.chanlist = (unsigned int __force *)user_chanlist;
cmd.data = NULL;
if (copy_to_user(arg, &cmd, sizeof(cmd))) {
dev_dbg(dev->class_dev, "fault writing cmd\n");
ret = -EFAULT;
goto cleanup;
}
ret = -EAGAIN;
goto cleanup;
}
This is a good place to make sure that async->cmd->chanlist is set,
perhaps by changing the code to something like this:
ret = s->do_cmdtest(dev, s, &async->cmd);
if (!ret && !async->cmd->chanlist) {
dev_dbg(dev->class_dev, "channel/gain list is NULL\n");
ret = -EINVAL;
}
if (async->cmd.flags & TRIG_BOGUS || ret) {
dev_dbg(dev->class_dev, "test returned %d\n", ret);
cmd = async->cmd;
/* restore chanlist pointer before copying back */
cmd.chanlist = (unsigned int __force *)user_chanlist;
cmd.data = NULL;
if (copy_to_user(arg, &cmd, sizeof(cmd))) {
dev_dbg(dev->class_dev, "fault writing cmd\n");
ret = -EFAULT;
}
if (ret >= 0)
ret = -EAGAIN;
goto cleanup;
}
(You could test user_chanlist instead of async->cmd->chanlist if you
like. You can also omit the dev_dbg() if you think it's superfluous.)
I was toying with the idea of setting ret = 5 instead of ret = -EINVAL
but think -EINVAL is better since the command is not fixable by trying
again. However, the code then needed changing to return something other
than -EAGAIN in that case, hence the `if (ret >= 0) ret = -EAGAIN` bit.
>
> diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci3120.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci3120.c
> index 66c24e4..c7323b5 100644
> --- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci3120.c
> +++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci3120.c
> @@ -777,8 +777,6 @@ static int apci3120_ai_cmdtest(struct comedi_device *dev,
> }
> }
>
> - err |= cfc_check_trigger_arg_min(&cmd->chanlist_len, 1);
> -
Yes, you can still get rid of these minimum value checks in the
do_cmdtest handlers.
> if (cmd->stop_src == TRIG_COUNT)
> err |= cfc_check_trigger_arg_min(&cmd->stop_arg, 1);
> else /* TRIG_NONE */
> diff --git a/drivers/staging/comedi/drivers/adl_pci9118.c b/drivers/staging/comedi/drivers/adl_pci9118.c
> index 53bbc59..465b304 100644
> --- a/drivers/staging/comedi/drivers/adl_pci9118.c
> +++ b/drivers/staging/comedi/drivers/adl_pci9118.c
> @@ -1258,10 +1258,7 @@ static int pci9118_ai_cmdtest(struct comedi_device *dev,
> else /* TRIG_NONE */
> err |= cfc_check_trigger_arg_is(&cmd->stop_arg, 0);
>
> - err |= cfc_check_trigger_arg_min(&cmd->chanlist_len, 1);
> -
> - err |= cfc_check_trigger_arg_min(&cmd->scan_end_arg,
> - cmd->chanlist_len);
> + err |= cfc_check_trigger_arg_min(&cmd->scan_end_arg, cmd->chanlist_len);
>
> if ((cmd->scan_end_arg % cmd->chanlist_len)) {
> cmd->scan_end_arg =
Since we allow cmd->chanlist_len to be 0, the above test needs changing to:
if (cmd->chanlist_len && (cmd->scan_end_arg % cmd->chanlist_len)) {
to avoid dividing by 0.
Also for consistency, near the end of pci9118_ai_cmdtest(), the test
if (cmd->chanlist)
should be changed to:
if (cmd->chanlist && cmd->chanlist_len)
(But if cmd->chanlist is forced to NULL by __comedi_get_user_chanlist()
when cmd->chanlist_len is 0, as mentioned in my remarks on
__comedi_get_user_chanlist() above, the simpler test is fine.)
> diff --git a/drivers/staging/comedi/drivers/amplc_pci224.c b/drivers/staging/comedi/drivers/amplc_pci224.c
> index 29e01e2..9f0c20f 100644
> --- a/drivers/staging/comedi/drivers/amplc_pci224.c
> +++ b/drivers/staging/comedi/drivers/amplc_pci224.c
> @@ -933,11 +933,6 @@ static int pci224_ao_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
> unsigned int rank;
> unsigned long flags;
>
> - /* Cannot handle null/empty chanlist. */
> - if (cmd->chanlist == NULL || cmd->chanlist_len == 0)
> - return -EINVAL;
> -
> -
> /* Determine which channels are enabled and their load order. */
> devpriv->ao_enab = 0;
>
> diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c b/drivers/staging/comedi/drivers/cb_pcidas64.c
> index f9afcbe..032bafe 100644
> --- a/drivers/staging/comedi/drivers/cb_pcidas64.c
> +++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
> @@ -2064,7 +2064,6 @@ static int ai_cmdtest(struct comedi_device *dev, struct comedi_subdevice *s,
> }
> }
>
> - err |= cfc_check_trigger_arg_min(&cmd->chanlist_len, 1);
> err |= cfc_check_trigger_arg_is(&cmd->scan_end_arg, cmd->chanlist_len);
>
> switch (cmd->stop_src) {
Near the bottom of ai_cmdtest() ("step 5") there's a test:
if (cmd->chanlist) {
that would need changing to:
if (cmd->chanlist && cmd->chanlist_len) {
unless the "force chanlist to NULL for zero chanlist_len" change I
mentioned earlier for __comedi_get_user_chanlist() is done. Otherwise,
the access to cmd->chanlist[0] within this `if` block would fail.
> @@ -3296,7 +3295,6 @@ static int ao_cmdtest(struct comedi_device *dev, struct comedi_subdevice *s,
> }
> }
>
> - err |= cfc_check_trigger_arg_min(&cmd->chanlist_len, 1);
> err |= cfc_check_trigger_arg_is(&cmd->scan_end_arg, cmd->chanlist_len);
>
> if (err)
And the same for ao_cmdtest() step 5 as for ai_cmdtest() step 5.
> diff --git a/drivers/staging/comedi/drivers/comedi_test.c b/drivers/staging/comedi/drivers/comedi_test.c
> index cd95625..0f7c844 100644
> --- a/drivers/staging/comedi/drivers/comedi_test.c
> +++ b/drivers/staging/comedi/drivers/comedi_test.c
> @@ -262,7 +262,6 @@ static int waveform_ai_cmdtest(struct comedi_device *dev,
> cmd->convert_arg * cmd->chanlist_len);
> }
>
> - err |= cfc_check_trigger_arg_min(&cmd->chanlist_len, 1);
> err |= cfc_check_trigger_arg_is(&cmd->scan_end_arg, cmd->chanlist_len);
>
> if (cmd->stop_src == TRIG_COUNT)
> diff --git a/drivers/staging/comedi/drivers/das1800.c b/drivers/staging/comedi/drivers/das1800.c
> index 8e975d6..c67f67a 100644
> --- a/drivers/staging/comedi/drivers/das1800.c
> +++ b/drivers/staging/comedi/drivers/das1800.c
> @@ -806,7 +806,6 @@ static int das1800_ai_do_cmdtest(struct comedi_device *dev,
> err |= cfc_check_trigger_arg_min(&cmd->convert_arg,
> thisboard->ai_speed);
>
> - err |= cfc_check_trigger_arg_min(&cmd->chanlist_len, 1);
> err |= cfc_check_trigger_arg_is(&cmd->scan_end_arg, cmd->chanlist_len);
>
> switch (cmd->stop_src) {
As for the cb_pcidas64 driver mentioned above, the test near the end of
das1800_ai_do_cmdtest() (step 5) needs changing to test both chanlist
and chanlist_len, unless chanlist gets forced to NULL by
__comedi_get_user_chanlist() when chanlist_len is 0. Otherwise the
access to cmd->chanlist[0] in the `if` block would fail.
> diff --git a/drivers/staging/comedi/drivers/das800.c b/drivers/staging/comedi/drivers/das800.c
> index 3e40837..ef5f65f 100644
> --- a/drivers/staging/comedi/drivers/das800.c
> +++ b/drivers/staging/comedi/drivers/das800.c
> @@ -338,7 +338,6 @@ static int das800_ai_do_cmdtest(struct comedi_device *dev,
> err |= cfc_check_trigger_arg_min(&cmd->convert_arg,
> thisboard->ai_speed);
>
> - err |= cfc_check_trigger_arg_min(&cmd->chanlist_len, 1);
> err |= cfc_check_trigger_arg_is(&cmd->scan_end_arg, cmd->chanlist_len);
>
> if (cmd->stop_src == TRIG_COUNT)
As for previously mentioned drivers, the test near the end of
das800_ai_do_cmdtest() (step 5) needs changing to test both chanlist and
chanlist_len, unless chanlist gets forced to NULL by
__comedi_get_user_chanlist() when chanlist_len is 0. Otherwise the
access to cmd->chanlist[0] in the `if` block would fail.
> diff --git a/drivers/staging/comedi/drivers/me4000.c b/drivers/staging/comedi/drivers/me4000.c
> index bd03a15..2657e5c 100644
> --- a/drivers/staging/comedi/drivers/me4000.c
> +++ b/drivers/staging/comedi/drivers/me4000.c
> @@ -605,18 +605,6 @@ static int ai_check_chanlist(struct comedi_device *dev,
> int aref;
> int i;
>
> - /* Check whether a channel list is available */
> - if (!cmd->chanlist_len) {
> - dev_err(dev->class_dev, "No channel list available\n");
> - return -EINVAL;
> - }
> -
> - /* Check the pointer */
> - if (!cmd->chanlist) {
> - dev_err(dev->class_dev, "NULL pointer to channel list\n");
> - return -EFAULT;
> - }
> -
> /* Check whether aref is equal for all entries */
> aref = CR_AREF(cmd->chanlist[0]);
> for (i = 0; i < cmd->chanlist_len; i++) {
> @@ -944,10 +932,6 @@ static int me4000_ai_do_cmd_test(struct comedi_device *dev,
>
> /* Step 3: check if arguments are trivially valid */
>
> - if (cmd->chanlist_len < 1) {
> - cmd->chanlist_len = 1;
> - err |= -EINVAL;
> - }
> if (init_ticks < 66) {
> cmd->start_arg = 2000;
> err |= -EINVAL;
The end of me4000_ai_do_cmd_test() (step 5) also needs changing to only
call ai_check_chanlist() if cmd->chanlist is non-NULL and
cmd->chanlist_len is non-0, something like:
if (cmd->chanlist && cmd->chanlist_len &&
ai_check_chanlist(dev, s, cmd))
return 5;
The test can be simplified if cmd->chanlist is forced to NULL by
__comedi_get_user_chanlist() when chanlist_len is 0, as mentioned earlier.
> diff --git a/drivers/staging/comedi/drivers/ni_at_a2150.c b/drivers/staging/comedi/drivers/ni_at_a2150.c
> index 4e39b1f..dc1c47c 100644
> --- a/drivers/staging/comedi/drivers/ni_at_a2150.c
> +++ b/drivers/staging/comedi/drivers/ni_at_a2150.c
> @@ -325,7 +325,6 @@ static int a2150_ai_cmdtest(struct comedi_device *dev,
> err |= cfc_check_trigger_arg_min(&cmd->convert_arg,
> thisboard->ai_speed);
>
> - err |= cfc_check_trigger_arg_min(&cmd->chanlist_len, 1);
> err |= cfc_check_trigger_arg_is(&cmd->scan_end_arg, cmd->chanlist_len);
>
> if (cmd->stop_src == TRIG_COUNT)
The test at the end of a2150_ai_cmdtest() (step 5) needs changing to
test both cmd->chanlist and cmd->chanlist_len, unless chanlist gets
forced to NULL by __comedi_get_user_chanlist() when chanlist_len is 0.
> diff --git a/drivers/staging/comedi/drivers/ni_labpc.c b/drivers/staging/comedi/drivers/ni_labpc.c
> index c33b9fc..b0754a2 100644
> --- a/drivers/staging/comedi/drivers/ni_labpc.c
> +++ b/drivers/staging/comedi/drivers/ni_labpc.c
> @@ -529,10 +529,6 @@ static enum scan_mode labpc_ai_scan_mode(const struct comedi_cmd *cmd)
> if (cmd->chanlist_len == 1)
> return MODE_SINGLE_CHAN;
>
> - /* chanlist may be NULL during cmdtest. */
> - if (cmd->chanlist == NULL)
> - return MODE_MULT_CHAN_UP;
> -
We'd need to keep that test in labpc_ai_scan_mode().
> if (CR_CHAN(cmd->chanlist[0]) == CR_CHAN(cmd->chanlist[1]))
> return MODE_SINGLE_CHAN_INTERVAL;
>
> @@ -552,9 +548,6 @@ static int labpc_ai_chanlist_invalid(const struct comedi_device *dev,
> {
> int channel, range, aref, i;
>
> - if (cmd->chanlist == NULL)
> - return 0;
> -
> if (mode == MODE_SINGLE_CHAN)
> return 0;
>
> @@ -656,8 +649,6 @@ static int labpc_ai_cmdtest(struct comedi_device *dev,
> if (cmd->start_arg == TRIG_NOW)
> err |= cfc_check_trigger_arg_is(&cmd->start_arg, 0);
>
> - if (!cmd->chanlist_len)
> - err |= -EINVAL;
> err |= cfc_check_trigger_arg_is(&cmd->scan_end_arg, cmd->chanlist_len);
>
> if (cmd->convert_src == TRIG_TIMER)
The end of labpc_ai_cmd_test() (step 5) also needs changing to only call
labpc_ai_chanlist_invalid() if cmd->chanlist is non-NULL and
cmd->chanlist_len is non-0, something like:
if (cmd->chanlist && cmd->chanlist_len &&
labpc_ai_chanlist_invalid(dev, cmd, mode))
return 5;
The test can be simplified if cmd->chanlist is forced to NULL by
__comedi_get_user_chanlist() when chanlist_len is 0, as mentioned earlier.
> diff --git a/drivers/staging/comedi/drivers/pcl812.c b/drivers/staging/comedi/drivers/pcl812.c
> index 0cf115e..1d4ec6e 100644
> --- a/drivers/staging/comedi/drivers/pcl812.c
> +++ b/drivers/staging/comedi/drivers/pcl812.c
> @@ -760,7 +760,6 @@ static int pcl812_ai_cmdtest(struct comedi_device *dev,
> else /* TRIG_EXT */
> err |= cfc_check_trigger_arg_is(&cmd->convert_arg, 0);
>
> - err |= cfc_check_trigger_arg_min(&cmd->chanlist_len, 1);
> err |= cfc_check_trigger_arg_is(&cmd->scan_end_arg, cmd->chanlist_len);
>
> if (cmd->stop_src == TRIG_COUNT)
> diff --git a/drivers/staging/comedi/drivers/pcl816.c b/drivers/staging/comedi/drivers/pcl816.c
> index 6f276f2..d4ead5e 100644
> --- a/drivers/staging/comedi/drivers/pcl816.c
> +++ b/drivers/staging/comedi/drivers/pcl816.c
> @@ -431,14 +431,10 @@ static int pcl816_ai_cmdtest(struct comedi_device *dev,
> if (err)
> return 4;
>
> + /* Step 5: check channel list */
>
> - /* step 5: complain about special chanlist considerations */
> -
> - if (cmd->chanlist) {
> - if (!check_channel_list(dev, s, cmd->chanlist,
> - cmd->chanlist_len))
> - return 5; /* incorrect channels list */
> - }
> + if (!check_channel_list(dev, s, cmd->chanlist, cmd->chanlist_len))
> + return 5;
We'd only want to call check_channel_list() if cmd->chanlist is non-NULL
and cmd->chanlist_len is non-0 as for the other drivers. But if
cmd->chanlist is forced to NULL by __comedi_get_user_chanlist() when
chanlist_len is 0, it is sufficient to test only cmd->chanlist.
>
> return 0;
> }
> diff --git a/drivers/staging/comedi/drivers/pcl818.c b/drivers/staging/comedi/drivers/pcl818.c
> index 6463476..36b8d22 100644
> --- a/drivers/staging/comedi/drivers/pcl818.c
> +++ b/drivers/staging/comedi/drivers/pcl818.c
> @@ -802,13 +802,10 @@ static int ai_cmdtest(struct comedi_device *dev, struct comedi_subdevice *s,
> if (err)
> return 4;
>
> - /* step 5: complain about special chanlist considerations */
> + /* Step 5: check channel list */
>
> - if (cmd->chanlist) {
> - if (!check_channel_list(dev, s, cmd->chanlist,
> - cmd->chanlist_len))
> - return 5; /* incorrect channels list */
> - }
> + if (!check_channel_list(dev, s, cmd->chanlist, cmd->chanlist_len))
> + return 5;
>
> return 0;
> }
Ditto as for pcl816.c, need to check both cmd->chanlist and
cmd->chanlist_len. But if cmd->chanlist is forced to NULL by
__comedi_get_user_chanlist() when chanlist_len is 0, it is sufficient to
test only cmd->chanlist.
> diff --git a/drivers/staging/comedi/drivers/usbduxfast.c b/drivers/staging/comedi/drivers/usbduxfast.c
> index d6fae11..9756110 100644
> --- a/drivers/staging/comedi/drivers/usbduxfast.c
> +++ b/drivers/staging/comedi/drivers/usbduxfast.c
> @@ -401,9 +401,6 @@ static int usbduxfast_ai_cmdtest(struct comedi_device *dev,
> if (cmd->start_src == TRIG_NOW)
> err |= cfc_check_trigger_arg_is(&cmd->start_arg, 0);
>
> - if (!cmd->chanlist_len)
> - err |= -EINVAL;
> -
> err |= cfc_check_trigger_arg_is(&cmd->scan_end_arg, cmd->chanlist_len);
>
> if (cmd->chanlist_len == 1)
>
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti at mev.co.uk> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
More information about the devel
mailing list