Re: [RFT PATCH] isdn: capi: Add check for controller count in detach_capi_ctr()

[Soumya Negi <soumya.negi97@gmail.com>]

On Sat, Jul 02, 2022 at 11:14:06AM +0300, Pavel Skripkin wrote:
> Hi Soumya,
> 
> Soumya Negi <soumya.negi97@gmail.com> says:
> > Fixes Syzbot bug:
> > https://syzkaller.appspot.com/bug?id=14f4820fbd379105a71fdee357b0759b90587a4e
> > 
> > This patch checks whether any ISDN devices are registered before unregistering
> > a CAPI controller(device). Without the check, the controller struct capi_str
> > results in out-of-bounds access bugs to other CAPI data strucures in
> > detach_capri_ctr() as seen in the bug report.
> > 
> > Reported-by: syzbot+9d567e08d3970bfd8271@syzkaller.appspotmail.com
> > 
> > Signed-off-by: Soumya Negi <soumya.negi97@gmail.com>
> > ---
> >   drivers/isdn/capi/kcapi.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/isdn/capi/kcapi.c b/drivers/isdn/capi/kcapi.c
> > index 18de41a266eb..6175ff7ec749 100644
> > --- a/drivers/isdn/capi/kcapi.c
> > +++ b/drivers/isdn/capi/kcapi.c
> > @@ -563,6 +563,9 @@ int detach_capi_ctr(struct capi_ctr *ctr)
> >   	mutex_lock(&capi_controller_lock);
> > +	if (ncontrollers == 0)
> > +		goto unlock_out;
> > +
> 
> It seems like to fix the problem. Did you mean to return 0 in case of
> ncontrollers == 0? Maybe it's better to return an error to indicate that
> function was called wrongly.

Yes, I let detach_capi_ctr() exit without an error code since I figured the
issue is caused by another subsystem in the first place.
But your logic sounds right. It is still an error and should be reflected
on return. I'll do that.
 
> On the other hand it means there are suspicious callers of that function.
> Maybe they should be fixed too.

It is being wrongly called without a controller count check from an external
function in the bluetooth stack's CMTP module. Should I fix the calling 
function in the same patch or go for a patchset?

> I'd put a warning in case of `ncontrollers == 0`, to indicate that something
> is going completely wrong.
 
Thanks for the quick reply,
Soumya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees