[v1] comedi: adl_pci9111: Fix possible division by zero in pci9111_ai_do_cmd_test()

[PATCH] comedi: adl_pci9111: Fix possible division by zero in pci9111_ai_do_cmd_test()

Posted by Aleksandr Mishin 1 year, 4 months ago

Division by zero is possible in pci9111_ai_do_cmd_test() in case of scan
begin trigger source is TRIG_TIMER and either 'chanlist_len' or
'convert_arg' is zero.

Add zero value check to prevent division by zero.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: f1c51faabc4d ("staging: comedi: adl_pci9111: tidy up (*do_cmdtest) Step 4")
Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru>
---
 drivers/comedi/drivers/adl_pci9111.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/comedi/drivers/adl_pci9111.c b/drivers/comedi/drivers/adl_pci9111.c
index 086d93f40cb9..ec1fb570b98c 100644
--- a/drivers/comedi/drivers/adl_pci9111.c
+++ b/drivers/comedi/drivers/adl_pci9111.c
@@ -312,6 +312,8 @@ static int pci9111_ai_do_cmd_test(struct comedi_device *dev,
 	 */
 	if (cmd->scan_begin_src == TRIG_TIMER) {
 		arg = cmd->chanlist_len * cmd->convert_arg;
+		if (!arg)
+			return 4;
 
 		if (arg < cmd->scan_begin_arg)
 			arg *= (cmd->scan_begin_arg / arg);
-- 
2.30.2

Re: [PATCH] comedi: adl_pci9111: Fix possible division by zero in pci9111_ai_do_cmd_test()

Posted by Ian Abbott 1 year, 4 months ago

On 18/09/2024 11:43, Aleksandr Mishin wrote:
> Division by zero is possible in pci9111_ai_do_cmd_test() in case of scan
> begin trigger source is TRIG_TIMER and either 'chanlist_len' or
> 'convert_arg' is zero.
> 
> Add zero value check to prevent division by zero.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: f1c51faabc4d ("staging: comedi: adl_pci9111: tidy up (*do_cmdtest) Step 4")
> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru>
> ---
>   drivers/comedi/drivers/adl_pci9111.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/comedi/drivers/adl_pci9111.c b/drivers/comedi/drivers/adl_pci9111.c
> index 086d93f40cb9..ec1fb570b98c 100644
> --- a/drivers/comedi/drivers/adl_pci9111.c
> +++ b/drivers/comedi/drivers/adl_pci9111.c
> @@ -312,6 +312,8 @@ static int pci9111_ai_do_cmd_test(struct comedi_device *dev,
>   	 */
>   	if (cmd->scan_begin_src == TRIG_TIMER) {
>   		arg = cmd->chanlist_len * cmd->convert_arg;
> +		if (!arg)
> +			return 4;
>   
>   		if (arg < cmd->scan_begin_arg)
>   			arg *= (cmd->scan_begin_arg / arg);

Nice catch!  `cmd->convert_arg` will be non-zero due to earlier checks, 
but `cmd->chanlist_len` could be zero for the `COMEDI_CMDTEST` ioctl. 
(The function is called for the `COMEDI_CMDTEST` and `COMEDI_CMD` 
ioctls, but only `COMEDI_CMD` checks that `chanlist_len` is non-zero 
before calling the function.)

Reviewed-by: Ian Abbott <abbotti@mev.co.uk>

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

Re: [PATCH] comedi: adl_pci9111: Fix possible division by zero in pci9111_ai_do_cmd_test()

Posted by Ian Abbott 1 year, 4 months ago

On 18/09/2024 12:41, Ian Abbott wrote:
> On 18/09/2024 11:43, Aleksandr Mishin wrote:
>> Division by zero is possible in pci9111_ai_do_cmd_test() in case of scan
>> begin trigger source is TRIG_TIMER and either 'chanlist_len' or
>> 'convert_arg' is zero.
>>
>> Add zero value check to prevent division by zero.
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Fixes: f1c51faabc4d ("staging: comedi: adl_pci9111: tidy up 
>> (*do_cmdtest) Step 4")
>> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru>
>> ---
>>   drivers/comedi/drivers/adl_pci9111.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/comedi/drivers/adl_pci9111.c 
>> b/drivers/comedi/drivers/adl_pci9111.c
>> index 086d93f40cb9..ec1fb570b98c 100644
>> --- a/drivers/comedi/drivers/adl_pci9111.c
>> +++ b/drivers/comedi/drivers/adl_pci9111.c
>> @@ -312,6 +312,8 @@ static int pci9111_ai_do_cmd_test(struct 
>> comedi_device *dev,
>>        */
>>       if (cmd->scan_begin_src == TRIG_TIMER) {
>>           arg = cmd->chanlist_len * cmd->convert_arg;
>> +        if (!arg)
>> +            return 4;
>>           if (arg < cmd->scan_begin_arg)
>>               arg *= (cmd->scan_begin_arg / arg);
> 
> Nice catch!  `cmd->convert_arg` will be non-zero due to earlier checks, 
> but `cmd->chanlist_len` could be zero for the `COMEDI_CMDTEST` ioctl. 
> (The function is called for the `COMEDI_CMDTEST` and `COMEDI_CMD` 
> ioctls, but only `COMEDI_CMD` checks that `chanlist_len` is non-zero 
> before calling the function.)
> 
> Reviewed-by: Ian Abbott <abbotti@mev.co.uk>

On second thoughts, the code should not return 4 (indicating a problem 
at checking step 4) here if `chanlist_len` is 0, because it is OK to use 
a zero `chanlist_len` value when using the `COMEDI_CMDTEST` ioctl.

Suggested fix is to keep your change, but to combine it with this change:

-	if (cmd->scan_begin_src == TRIG_TIMER) {
+	if (cmd->scan_begin_src == TRIG_TIMER && cmd->chanlist_len) {

I think `cmd->convert_arg` will be non-zero unless the call to 
`comedi_8254_cascade_ns_to_timer()` somehow manages to set it to zero.

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

[PATCH v2] comedi: adl_pci9111: Fix possible division by zero in pci9111_ai_do_cmd_test()

Posted by Aleksandr Mishin 1 year, 4 months ago

Division by zero is possible in pci9111_ai_do_cmd_test() in case of scan
begin trigger source is TRIG_TIMER and 'chanlist_len' is zero.

Add zero value check to prevent division by zero.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: f1c51faabc4d ("staging: comedi: adl_pci9111: tidy up (*do_cmdtest) Step 4")
Suggested-by: Ian Abbott <abbotti@mev.co.uk>
Reviewed-by: Ian Abbott <abbotti@mev.co.uk>
Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru>
---
v1->v2: Update comment and fix as suggested by Ian,
 add "Reviewed-by: Ian Abbott <abbotti@mev.co.uk>"
 (https://lore.kernel.org/all/4f46343a-a1f9-4082-8ef2-50cdb3d74f31@mev.co.uk/)

 drivers/comedi/drivers/adl_pci9111.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/comedi/drivers/adl_pci9111.c b/drivers/comedi/drivers/adl_pci9111.c
index 086d93f40cb9..e5989d180650 100644
--- a/drivers/comedi/drivers/adl_pci9111.c
+++ b/drivers/comedi/drivers/adl_pci9111.c
@@ -310,7 +310,7 @@ static int pci9111_ai_do_cmd_test(struct comedi_device *dev,
 	 * There's only one timer on this card, so the scan_begin timer
 	 * must be a multiple of chanlist_len*convert_arg
 	 */
-	if (cmd->scan_begin_src == TRIG_TIMER) {
+	if (cmd->scan_begin_src == TRIG_TIMER && cmd->chanlist_len) {
 		arg = cmd->chanlist_len * cmd->convert_arg;
 
 		if (arg < cmd->scan_begin_arg)
-- 
2.30.2