Re: [PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest
On Wed, Mar 13, 2019 at 06:57:17PM +, 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
> Tested-by: Ivan Vasilyev
Not too late, I'll go add it now, thanks.
greg k-h
Re: [PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest
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
Tested-by: Ivan Vasilyev
Cc: # 4.6+
Cc: Spencer E. Olson
Signed-off-by: Ian Abbott
---
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 || 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. )=-
Re: [PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest
On 05/03/2019 11:39, Dan Carpenter wrote: On Tue, Mar 05, 2019 at 11:32:18AM +, Ian Abbott wrote: On 05/03/2019 11:10, Dan Carpenter wrote: On Mon, Mar 04, 2019 at 02:33:54PM +, 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 || 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. )=-
Re: [PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest
On Tue, Mar 05, 2019 at 11:32:18AM +, Ian Abbott wrote: > On 05/03/2019 11:10, Dan Carpenter wrote: > > On Mon, Mar 04, 2019 at 02:33:54PM +, 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
Re: [PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest
On 05/03/2019 11:10, Dan Carpenter wrote:
On Mon, Mar 04, 2019 at 02:33:54PM +, 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: # 4.6+
Cc: Spencer E. Olson
Signed-off-by: Ian Abbott
---
regards,
dan carpenter
--
-=( Ian Abbott || 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. )=-
Re: [PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest
On Mon, Mar 04, 2019 at 02:33:54PM +, 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: # 4.6+
> Cc: Spencer E. Olson
> Signed-off-by: Ian Abbott
> ---
regards,
dan carpenter
[PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest
`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: # 4.6+
Cc: Spencer E. Olson
Signed-off-by: Ian Abbott
---
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 = >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
+ *
