[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <04320187-02bb-440c-9180-c1198dddecb3@mev.co.uk>
Date: Wed, 18 Sep 2024 13:09:04 +0100
From: Ian Abbott <abbotti@....co.uk>
To: Aleksandr Mishin <amishin@...rgos.ru>,
H Hartley Sweeten <hsweeten@...ionengravers.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-kernel@...r.kernel.org, lvc-project@...uxtesting.org
Subject: Re: [PATCH] comedi: adl_pci9111: Fix possible division by zero in
pci9111_ai_do_cmd_test()
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@...rgos.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@....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@....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 )=-
Powered by blists - more mailing lists