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

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

[Soumya Negi]

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;
+
 	ctr_down(ctr, CAPI_CTR_DETACHED);
 
 	if (capi_controller[ctr->cnr - 1] != ctr) {
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Pavel Skripkin]

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.


On the other hand it means there are suspicious callers of that 
function. Maybe they should be fixed too.

I'd put a warning in case of `ncontrollers == 0`, to indicate that 
something is going completely wrong.




Thanks,
--Pavel Skripkin
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Soumya Negi]

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

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

[Soumya Negi]

Hi,
I was just told on the syzkaller mailing list that this bug had been
fixed long back and syzbot didn't catch it as the commit didn't have
a Fixes tag.

-Soumya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Shuah Khan]

On 7/4/22 9:03 PM, Soumya Negi wrote:
> Hi,
> I was just told on the syzkaller mailing list that this bug had been
> fixed long back and syzbot didn't catch it as the commit didn't have
> a Fixes tag.
> 
> -Soumya
> 

You can check to see how the problem was fixed and if it is different
from your fix. Might help in fixing other such problems in the future.

thanks,
-- Shuah
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees