Re: [PATCH] commdi: pcl726: Prevent invalid irq number

[Ian Abbott <abbotti@mev.co.uk>]

On 07/07/2025 10:47, Ian Abbott wrote:
> On 06/07/2025 03:10, Edward Adam Davis wrote:
> 
> The "comedi" tag in the subject line is misspelled.
> 
>> The reproducer passed in an irq number(0x80008000) that was too large,
>> which triggered the oob.
>>
>> Added an interrupt number check to prevent users from passing in an irq
>> number that was too large.
>>
>> Fixes: fff46207245c ("staging: comedi: pcl726: enable the interrupt 
>> support code")
>> Reported-by: syzbot+5cd373521edd68bebcb3@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=5cd373521edd68bebcb3
>> Tested-by: syzbot+5cd373521edd68bebcb3@syzkaller.appspotmail.com
> 
> Thanks.
> 
> You could add:
> 
> Cc: <stable@vger.kernel.org> # 5.13+
> 
> to apply it to stable kernels.  (The 5.13+ is because comedi was moved 
> out of the "staging" directory in 5.13, and a backport would be required 
> for longterm series < 5.13.)
> 
> More comments below...
> 
>> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
>> ---
>>   drivers/comedi/drivers/pcl726.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/comedi/drivers/pcl726.c b/drivers/comedi/drivers/ 
>> pcl726.c
>> index 0430630e6ebb..8802f33f1954 100644
>> --- a/drivers/comedi/drivers/pcl726.c
>> +++ b/drivers/comedi/drivers/pcl726.c
>> @@ -328,6 +328,9 @@ static int pcl726_attach(struct comedi_device *dev,
>>        * Hook up the external trigger source interrupt only if the
>>        * user config option is valid and the board supports interrupts.
>>        */
>> +    if (it->options[1] < 0 || it->options[1] > 31)
>> +        return -EINVAL;
>> +

I missed this one earlier, but returning `-EINVAL` changes the behavior. 
  The old code would just not attempt to request the IRQ if the 
`options[1]` value were invalid.  And it would still configure the 
device without interrupts even if the call to `request_irq` returned an 
error.  So it would be better to combine this test with the test below.

>>       if (it->options[1] && (board->irq_mask & (1 << it->options[1]))) {
>>           ret = request_irq(it->options[1], pcl726_interrupt, 0,
>>                     dev->board_name, dev);
> 
> If `it->options[1]` is 31, then `1 << it->options[1]` is still invalid 
> because it shifts a 1-bit into the sign bit (which is UB in C). Possible 
> solutions include reducing the upper bound on the `it->options[1]` value 
> to 30 or lower, or using `1U << it->options[1]`.
> 

For example:

	if (it->options[1] > 0 && it->options[1] < 16 &&
             (board->irq_mask & (1U << it->options[1])) {

Thanks.

-- 
-=( 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 )=-