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

[Pavel Skripkin <paskripkin@gmail.com>]

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