* [PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest
@ 2019-03-04 14:33 Ian Abbott
2019-03-05 11:10 ` Dan Carpenter
2019-03-13 18:57 ` Ian Abbott
0 siblings, 2 replies; 7+ messages in thread
From: Ian Abbott @ 2019-03-04 14:33 UTC (permalink / raw)
To: devel
Cc: Greg Kroah-Hartman, Ian Abbott, H Hartley Sweeten, linux-kernel,
stable, Spencer E . Olson
`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/#!topic/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;
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest
2019-03-04 14:33 [PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest Ian Abbott
@ 2019-03-05 11:10 ` Dan Carpenter
2019-03-05 11:32 ` Ian Abbott
2019-03-13 18:57 ` Ian Abbott
1 sibling, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2019-03-05 11:10 UTC (permalink / raw)
To: Ian Abbott
Cc: devel, Greg Kroah-Hartman, linux-kernel, stable, Spencer E . Olson
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/#!topic/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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest
2019-03-05 11:10 ` Dan Carpenter
@ 2019-03-05 11:32 ` Ian Abbott
2019-03-05 11:39 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Ian Abbott @ 2019-03-05 11:32 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, Greg Kroah-Hartman, linux-kernel, stable, Spencer E . Olson
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/#!topic/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
>
>
--
-=( Ian Abbott <abbotti@mev.co.uk> || 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. )=-
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest
2019-03-05 11:32 ` Ian Abbott
@ 2019-03-05 11:39 ` Dan Carpenter
2019-03-05 12:04 ` Ian Abbott
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2019-03-05 11:39 UTC (permalink / raw)
To: Ian Abbott
Cc: devel, Greg Kroah-Hartman, Spencer E . Olson, linux-kernel, stable
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/#!topic/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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest
2019-03-05 11:39 ` Dan Carpenter
@ 2019-03-05 12:04 ` Ian Abbott
0 siblings, 0 replies; 7+ messages in thread
From: Ian Abbott @ 2019-03-05 12:04 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, Greg Kroah-Hartman, Spencer E . Olson, linux-kernel, stable
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/#!topic/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.
--
-=( Ian Abbott <abbotti@mev.co.uk> || 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. )=-
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest
2019-03-04 14:33 [PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest Ian Abbott
2019-03-05 11:10 ` Dan Carpenter
@ 2019-03-13 18:57 ` Ian Abbott
2019-03-18 6:54 ` Greg Kroah-Hartman
1 sibling, 1 reply; 7+ messages in thread
From: Ian Abbott @ 2019-03-13 18:57 UTC (permalink / raw)
To: devel
Cc: Greg Kroah-Hartman, H Hartley Sweeten, linux-kernel, stable,
Spencer E . Olson
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/#!topic/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(-)
--
-=( Ian Abbott <abbotti@mev.co.uk> || 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. )=-
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest
2019-03-13 18:57 ` Ian Abbott
@ 2019-03-18 6:54 ` Greg Kroah-Hartman
0 siblings, 0 replies; 7+ messages in thread
From: Greg Kroah-Hartman @ 2019-03-18 6:54 UTC (permalink / raw)
To: Ian Abbott
Cc: devel, H Hartley Sweeten, linux-kernel, stable, Spencer E . Olson
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/#!topic/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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-03-18 6:54 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-04 14:33 [PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest Ian Abbott
2019-03-05 11:10 ` Dan Carpenter
2019-03-05 11:32 ` Ian Abbott
2019-03-05 11:39 ` Dan Carpenter
2019-03-05 12:04 ` Ian Abbott
2019-03-13 18:57 ` Ian Abbott
2019-03-18 6:54 ` Greg Kroah-Hartman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).