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

[Ian Abbott]

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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://syzkaller.appspot.com/bug?extid=5cd373521edd68bebcb3
> > Tested-by: syzbot+5cd373521edd68bebcb3@xxxxxxxxxxxxxxxxxxxxxxxxx
>
> Thanks.
>
> You could add:
>
> Cc: <stable@xxxxxxxxxxxxxxx> # 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@xxxxxx>
> > ---
> >   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@xxxxxxxxx> || 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 )=-