`ni_cdio_cmdtest()` validates Comedi asynchronous commands for the DIO subdevice (subdevice 2) of supported National Instruments M-series cards. It is called when handling the `COMEDI_CMD` and `COMEDI_CMDTEST` ioctls for this subdevice. There are two causes for a possible divide-by-zero error when validating that the `stop_arg` member of the passed-in command is not too large.
The first cause for the divide-by-zero is that calls to `comedi_bytes_per_scan()` are only valid once the command has been copied to `s->async->cmd`, but that copy is only done for the `COMEDI_CMD` ioctl. For the `COMEDI_CMDTEST` ioctl, it will use whatever was left there by the previous `COMEDI_CMD` ioctl, if any. (This is very likely, as it is usual for the application to use `COMEDI_CMDTEST` before `COMEDI_CMD`.) If there has been no previous, valid `COMEDI_CMD` for this subdevice, then `comedi_bytes_per_scan()` will return 0, so the subsequent division in `ni_cdio_cmdtest()` of `s->async->prealloc_bufsz / comedi_bytes_per_scan(s)` will be a divide-by-zero error. To fix this error, call a new function `comedi_bytes_per_scan_cmd(s, cmd)`, based on the existing `comedi_bytes_per_scan(s)` but using a specified `struct comedi_cmd` for its calculations. (Also refactor `comedi_bytes_per_scan()` to call the new function.)
Once the first cause for the divide-by-zero has been fixed, the second cause is that `comedi_bytes_per_scan_cmd()` can legitimately return 0 if the `scan_end_arg` member of the `struct comedi_cmd` being tested is 0. Fix it by only performing the division (and validating that `stop_arg` is no more than the maximum value) if `comedi_bytes_per_scan_cmd()` returns a non-zero value.
The problem was reported on the COMEDI mailing list here: https://groups.google.com/forum/#%21topic/comedi_list/4t9WlHzMhKM
Fixes: f164cbf98fa8 ("staging: comedi: ni_mio_common: add finite regeneration to dio output") Cc: stable@vger.kernel.org # 4.6+ Cc: Spencer E. Olson olsonse@umich.edu Signed-off-by: Ian Abbott abbotti@mev.co.uk --- drivers/staging/comedi/comedidev.h | 2 ++ drivers/staging/comedi/drivers.c | 33 ++++++++++++++++--- .../staging/comedi/drivers/ni_mio_common.c | 10 ++++-- 3 files changed, 38 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/comedi/comedidev.h b/drivers/staging/comedi/comedidev.h index a7d569cfca5d..0dff1ac057cd 100644 --- a/drivers/staging/comedi/comedidev.h +++ b/drivers/staging/comedi/comedidev.h @@ -1001,6 +1001,8 @@ int comedi_dio_insn_config(struct comedi_device *dev, unsigned int mask); unsigned int comedi_dio_update_state(struct comedi_subdevice *s, unsigned int *data); +unsigned int comedi_bytes_per_scan_cmd(struct comedi_subdevice *s, + struct comedi_cmd *cmd); unsigned int comedi_bytes_per_scan(struct comedi_subdevice *s); unsigned int comedi_nscans_left(struct comedi_subdevice *s, unsigned int nscans); diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c index eefa62f42c0f..5a32b8fc000e 100644 --- a/drivers/staging/comedi/drivers.c +++ b/drivers/staging/comedi/drivers.c @@ -394,11 +394,13 @@ unsigned int comedi_dio_update_state(struct comedi_subdevice *s, EXPORT_SYMBOL_GPL(comedi_dio_update_state);
/** - * comedi_bytes_per_scan() - Get length of asynchronous command "scan" in bytes + * comedi_bytes_per_scan_cmd() - Get length of asynchronous command "scan" in + * bytes * @s: COMEDI subdevice. + * @cmd: COMEDI command. * * Determines the overall scan length according to the subdevice type and the - * number of channels in the scan. + * number of channels in the scan for the specified command. * * For digital input, output or input/output subdevices, samples for * multiple channels are assumed to be packed into one or more unsigned @@ -408,9 +410,9 @@ EXPORT_SYMBOL_GPL(comedi_dio_update_state); * * Returns the overall scan length in bytes. */ -unsigned int comedi_bytes_per_scan(struct comedi_subdevice *s) +unsigned int comedi_bytes_per_scan_cmd(struct comedi_subdevice *s, + struct comedi_cmd *cmd) { - struct comedi_cmd *cmd = &s->async->cmd; unsigned int num_samples; unsigned int bits_per_sample;
@@ -427,6 +429,29 @@ unsigned int comedi_bytes_per_scan(struct comedi_subdevice *s) } return comedi_samples_to_bytes(s, num_samples); } +EXPORT_SYMBOL_GPL(comedi_bytes_per_scan_cmd); + +/** + * comedi_bytes_per_scan() - Get length of asynchronous command "scan" in bytes + * @s: COMEDI subdevice. + * + * Determines the overall scan length according to the subdevice type and the + * number of channels in the scan for the current command. + * + * For digital input, output or input/output subdevices, samples for + * multiple channels are assumed to be packed into one or more unsigned + * short or unsigned int values according to the subdevice's %SDF_LSAMPL + * flag. For other types of subdevice, samples are assumed to occupy a + * whole unsigned short or unsigned int according to the %SDF_LSAMPL flag. + * + * Returns the overall scan length in bytes. + */ +unsigned int comedi_bytes_per_scan(struct comedi_subdevice *s) +{ + struct comedi_cmd *cmd = &s->async->cmd; + + return comedi_bytes_per_scan_cmd(s, cmd); +} EXPORT_SYMBOL_GPL(comedi_bytes_per_scan);
static unsigned int __comedi_nscans_left(struct comedi_subdevice *s, diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c index 5edf59ac6706..b04dad8c7092 100644 --- a/drivers/staging/comedi/drivers/ni_mio_common.c +++ b/drivers/staging/comedi/drivers/ni_mio_common.c @@ -3545,6 +3545,7 @@ static int ni_cdio_cmdtest(struct comedi_device *dev, struct comedi_subdevice *s, struct comedi_cmd *cmd) { struct ni_private *devpriv = dev->private; + unsigned int bytes_per_scan; int err = 0;
/* Step 1 : check if triggers are trivially valid */ @@ -3579,9 +3580,12 @@ static int ni_cdio_cmdtest(struct comedi_device *dev, err |= comedi_check_trigger_arg_is(&cmd->convert_arg, 0); err |= comedi_check_trigger_arg_is(&cmd->scan_end_arg, cmd->chanlist_len); - err |= comedi_check_trigger_arg_max(&cmd->stop_arg, - s->async->prealloc_bufsz / - comedi_bytes_per_scan(s)); + bytes_per_scan = comedi_bytes_per_scan_cmd(s, cmd); + if (bytes_per_scan) { + err |= comedi_check_trigger_arg_max(&cmd->stop_arg, + s->async->prealloc_bufsz / + bytes_per_scan); + }
if (err) return 3;
On Mon, Mar 04, 2019 at 02:33:54PM +0000, Ian Abbott wrote:
`ni_cdio_cmdtest()` validates Comedi asynchronous commands for the DIO subdevice (subdevice 2) of supported National Instruments M-series cards. It is called when handling the `COMEDI_CMD` and `COMEDI_CMDTEST` ioctls for this subdevice. There are two causes for a possible divide-by-zero error when validating that the `stop_arg` member of the passed-in command is not too large.
The first cause for the divide-by-zero is that calls to `comedi_bytes_per_scan()` are only valid once the command has been copied to `s->async->cmd`, but that copy is only done for the `COMEDI_CMD` ioctl. For the `COMEDI_CMDTEST` ioctl, it will use whatever was left there by the previous `COMEDI_CMD` ioctl, if any. (This is very likely, as it is usual for the application to use `COMEDI_CMDTEST` before `COMEDI_CMD`.) If there has been no previous, valid `COMEDI_CMD` for this subdevice, then `comedi_bytes_per_scan()` will return 0, so the subsequent division in `ni_cdio_cmdtest()` of `s->async->prealloc_bufsz / comedi_bytes_per_scan(s)` will be a divide-by-zero error. To fix this error, call a new function `comedi_bytes_per_scan_cmd(s, cmd)`, based on the existing `comedi_bytes_per_scan(s)` but using a specified `struct comedi_cmd` for its calculations. (Also refactor `comedi_bytes_per_scan()` to call the new function.)
Once the first cause for the divide-by-zero has been fixed, the second cause is that `comedi_bytes_per_scan_cmd()` can legitimately return 0 if the `scan_end_arg` member of the `struct comedi_cmd` being tested is 0. Fix it by only performing the division (and validating that `stop_arg` is no more than the maximum value) if `comedi_bytes_per_scan_cmd()` returns a non-zero value.
The problem was reported on the COMEDI mailing list here: https://groups.google.com/forum/#%21topic/comedi_list/4t9WlHzMhKM
Can you give Ivan a Reported-by tag? It's a public mailing list so that shouldn't be a problem.
Fixes: f164cbf98fa8 ("staging: comedi: ni_mio_common: add finite regeneration to dio output") Cc: stable@vger.kernel.org # 4.6+ Cc: Spencer E. Olson olsonse@umich.edu Signed-off-by: Ian Abbott abbotti@mev.co.uk
regards, dan carpenter
On 05/03/2019 11:10, Dan Carpenter wrote:
On Mon, Mar 04, 2019 at 02:33:54PM +0000, Ian Abbott wrote:
The problem was reported on the COMEDI mailing list here: https://groups.google.com/forum/#%21topic/comedi_list/4t9WlHzMhKM
Can you give Ivan a Reported-by tag? It's a public mailing list so that shouldn't be a problem.
I can do, but I don't know his full name. Will that be a problem?
Fixes: f164cbf98fa8 ("staging: comedi: ni_mio_common: add finite regeneration to dio output") Cc: stable@vger.kernel.org # 4.6+ Cc: Spencer E. Olson olsonse@umich.edu Signed-off-by: Ian Abbott abbotti@mev.co.uk
regards, dan carpenter
On Tue, Mar 05, 2019 at 11:32:18AM +0000, Ian Abbott wrote:
On 05/03/2019 11:10, Dan Carpenter wrote:
On Mon, Mar 04, 2019 at 02:33:54PM +0000, Ian Abbott wrote:
The problem was reported on the COMEDI mailing list here: https://groups.google.com/forum/#%21topic/comedi_list/4t9WlHzMhKM
Can you give Ivan a Reported-by tag? It's a public mailing list so that shouldn't be a problem.
I can do, but I don't know his full name. Will that be a problem?
Nah, it's not a problem. People should fix their email headers to reflect what they want to be called.
Or you could ask but I don't think I have ever asked for someone's name I've always just gone with their email header name.
regards, dan carpenter
On 05/03/2019 11:39, Dan Carpenter wrote:
On Tue, Mar 05, 2019 at 11:32:18AM +0000, Ian Abbott wrote:
On 05/03/2019 11:10, Dan Carpenter wrote:
On Mon, Mar 04, 2019 at 02:33:54PM +0000, Ian Abbott wrote:
The problem was reported on the COMEDI mailing list here: https://groups.google.com/forum/#%21topic/comedi_list/4t9WlHzMhKM
Can you give Ivan a Reported-by tag? It's a public mailing list so that shouldn't be a problem.
I can do, but I don't know his full name. Will that be a problem?
Nah, it's not a problem. People should fix their email headers to reflect what they want to be called.
Or you could ask but I don't think I have ever asked for someone's name I've always just gone with their email header name.
In this case, Ivan just signed off with that name and it didn't appear in the email headers.
On 04/03/2019 14:33, Ian Abbott wrote:
`ni_cdio_cmdtest()` validates Comedi asynchronous commands for the DIO subdevice (subdevice 2) of supported National Instruments M-series cards. It is called when handling the `COMEDI_CMD` and `COMEDI_CMDTEST` ioctls for this subdevice. There are two causes for a possible divide-by-zero error when validating that the `stop_arg` member of the passed-in command is not too large.
The first cause for the divide-by-zero is that calls to `comedi_bytes_per_scan()` are only valid once the command has been copied to `s->async->cmd`, but that copy is only done for the `COMEDI_CMD` ioctl. For the `COMEDI_CMDTEST` ioctl, it will use whatever was left there by the previous `COMEDI_CMD` ioctl, if any. (This is very likely, as it is usual for the application to use `COMEDI_CMDTEST` before `COMEDI_CMD`.) If there has been no previous, valid `COMEDI_CMD` for this subdevice, then `comedi_bytes_per_scan()` will return 0, so the subsequent division in `ni_cdio_cmdtest()` of `s->async->prealloc_bufsz / comedi_bytes_per_scan(s)` will be a divide-by-zero error. To fix this error, call a new function `comedi_bytes_per_scan_cmd(s, cmd)`, based on the existing `comedi_bytes_per_scan(s)` but using a specified `struct comedi_cmd` for its calculations. (Also refactor `comedi_bytes_per_scan()` to call the new function.)
Once the first cause for the divide-by-zero has been fixed, the second cause is that `comedi_bytes_per_scan_cmd()` can legitimately return 0 if the `scan_end_arg` member of the `struct comedi_cmd` being tested is 0. Fix it by only performing the division (and validating that `stop_arg` is no more than the maximum value) if `comedi_bytes_per_scan_cmd()` returns a non-zero value.
The problem was reported on the COMEDI mailing list here: https://groups.google.com/forum/#%21topic/comedi_list/4t9WlHzMhKM
Fixes: f164cbf98fa8 ("staging: comedi: ni_mio_common: add finite regeneration to dio output")
Greg, If it's not too late, it would be nice if the following "Reported-by:" and "Tested-by:" lines could be added (or I can resend with these lines included if necessary). It's no big deal if this is too late. I'll live with it. Thanks.
Reported-by: Ivan Vasilyev grabesstimme@gmail.com Tested-by: Ivan Vasilyev grabesstimme@gmail.com
Cc: stable@vger.kernel.org # 4.6+ Cc: Spencer E. Olson olsonse@umich.edu Signed-off-by: Ian Abbott abbotti@mev.co.uk
drivers/staging/comedi/comedidev.h | 2 ++ drivers/staging/comedi/drivers.c | 33 ++++++++++++++++--- .../staging/comedi/drivers/ni_mio_common.c | 10 ++++-- 3 files changed, 38 insertions(+), 7 deletions(-)
On Wed, Mar 13, 2019 at 06:57:17PM +0000, Ian Abbott wrote:
On 04/03/2019 14:33, Ian Abbott wrote:
`ni_cdio_cmdtest()` validates Comedi asynchronous commands for the DIO subdevice (subdevice 2) of supported National Instruments M-series cards. It is called when handling the `COMEDI_CMD` and `COMEDI_CMDTEST` ioctls for this subdevice. There are two causes for a possible divide-by-zero error when validating that the `stop_arg` member of the passed-in command is not too large.
The first cause for the divide-by-zero is that calls to `comedi_bytes_per_scan()` are only valid once the command has been copied to `s->async->cmd`, but that copy is only done for the `COMEDI_CMD` ioctl. For the `COMEDI_CMDTEST` ioctl, it will use whatever was left there by the previous `COMEDI_CMD` ioctl, if any. (This is very likely, as it is usual for the application to use `COMEDI_CMDTEST` before `COMEDI_CMD`.) If there has been no previous, valid `COMEDI_CMD` for this subdevice, then `comedi_bytes_per_scan()` will return 0, so the subsequent division in `ni_cdio_cmdtest()` of `s->async->prealloc_bufsz / comedi_bytes_per_scan(s)` will be a divide-by-zero error. To fix this error, call a new function `comedi_bytes_per_scan_cmd(s, cmd)`, based on the existing `comedi_bytes_per_scan(s)` but using a specified `struct comedi_cmd` for its calculations. (Also refactor `comedi_bytes_per_scan()` to call the new function.)
Once the first cause for the divide-by-zero has been fixed, the second cause is that `comedi_bytes_per_scan_cmd()` can legitimately return 0 if the `scan_end_arg` member of the `struct comedi_cmd` being tested is 0. Fix it by only performing the division (and validating that `stop_arg` is no more than the maximum value) if `comedi_bytes_per_scan_cmd()` returns a non-zero value.
The problem was reported on the COMEDI mailing list here: https://groups.google.com/forum/#%21topic/comedi_list/4t9WlHzMhKM
Fixes: f164cbf98fa8 ("staging: comedi: ni_mio_common: add finite regeneration to dio output")
Greg, If it's not too late, it would be nice if the following "Reported-by:" and "Tested-by:" lines could be added (or I can resend with these lines included if necessary). It's no big deal if this is too late. I'll live with it. Thanks.
Reported-by: Ivan Vasilyev grabesstimme@gmail.com Tested-by: Ivan Vasilyev grabesstimme@gmail.com
Not too late, I'll go add it now, thanks.
greg k-h
linux-stable-mirror@lists.linaro.org
-
Dan Carpenter -
Greg Kroah-Hartman -
Ian Abbott