[PATCH] isdn: mISDN: tei: Fix a sleep-in-atomic-context bug in create_teimgr()

[PATCH] isdn: mISDN: tei: Fix a sleep-in-atomic-context bug in create_teimgr()

[Jia-Ju Bai]

The kernel module may sleep with holding a spinlock.

The function call paths (from bottom to top) in Linux-4.16 are:

[FUNC] kzalloc(GFP_KERNEL)
drivers/isdn/mISDN/tei.c, 1058: kzalloc in create_teimgr
drivers/isdn/mISDN/tei.c, 1278: create_teimgr in mgr_ctrl
drivers/isdn/mISDN/tei.c, 1048: [FUNC_PTR]mgr_ctrl in create_teimgr
drivers/isdn/mISDN/tei.c, 1045: _raw_read_lock_irqsave in create_teimgr

Note that [FUNC_PTR] means a function pointer call is used.

To fix this bug, GFP_KERNEL is replaced with GFP_ATOMIC.

This bug is found by my static analysis tool DSAC.

Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
 drivers/isdn/mISDN/tei.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/isdn/mISDN/tei.c b/drivers/isdn/mISDN/tei.c
index 12d9e5f4beb1..6d95ee639fdb 100644
--- a/drivers/isdn/mISDN/tei.c
+++ b/drivers/isdn/mISDN/tei.c
@@ -1055,7 +1055,7 @@ create_teimgr(struct manager *mgr, struct channel_req *crq)
 		       crq->adr.tei, crq->adr.sapi);
 	if (!l2)
 		return -ENOMEM;
-	l2->tm = kzalloc(sizeof(struct teimgr), GFP_KERNEL);
+	l2->tm = kzalloc(sizeof(struct teimgr), GFP_ATOMIC);
 	if (!l2->tm) {
 		kfree(l2);
 		printk(KERN_ERR "kmalloc teimgr failed\n");
-- 
2.17.0

Re: [PATCH] isdn: mISDN: tei: Fix a sleep-in-atomic-context bug in create_teimgr()

[isdn]

Hi,

I do not understand the analysis and do not see that the spinlock is a
problem here.
I think your DSAC analyzer assumes that the FUNC_PTR mgr_ctrl call calls
the  mgr_ctrl in tei.c, but in real it calls l2->ch.ctrl() which is the
function in layer2.c, not tei.c. And the function in layer2.c should not
do any GFP_KERNEL allocation.

Same for your 2. reported issue.

Am 01.09.2018 um 14:00 schrieb Jia-Ju Bai:
> The kernel module may sleep with holding a spinlock.
> 
> The function call paths (from bottom to top) in Linux-4.16 are:
> 
> [FUNC] kzalloc(GFP_KERNEL)
> drivers/isdn/mISDN/tei.c, 1058: kzalloc in create_teimgr
> drivers/isdn/mISDN/tei.c, 1278: create_teimgr in mgr_ctrl
> drivers/isdn/mISDN/tei.c, 1048: [FUNC_PTR]mgr_ctrl in create_teimgr
> drivers/isdn/mISDN/tei.c, 1045: _raw_read_lock_irqsave in create_teimgr
> 
> Note that [FUNC_PTR] means a function pointer call is used.
> 
> To fix this bug, GFP_KERNEL is replaced with GFP_ATOMIC.
> 
> This bug is found by my static analysis tool DSAC.
> 
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> ---
>  drivers/isdn/mISDN/tei.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/isdn/mISDN/tei.c b/drivers/isdn/mISDN/tei.c
> index 12d9e5f4beb1..6d95ee639fdb 100644
> --- a/drivers/isdn/mISDN/tei.c
> +++ b/drivers/isdn/mISDN/tei.c
> @@ -1055,7 +1055,7 @@ create_teimgr(struct manager *mgr, struct channel_req *crq)
>  		       crq->adr.tei, crq->adr.sapi);
>  	if (!l2)
>  		return -ENOMEM;
> -	l2->tm = kzalloc(sizeof(struct teimgr), GFP_KERNEL);
> +	l2->tm = kzalloc(sizeof(struct teimgr), GFP_ATOMIC);
>  	if (!l2->tm) {
>  		kfree(l2);
>  		printk(KERN_ERR "kmalloc teimgr failed\n");
> 

Re: [PATCH] isdn: mISDN: tei: Fix a sleep-in-atomic-context bug in create_teimgr()

[Jia-Ju Bai]

On 2018/9/3 0:31, isdn@linux-pingi.de wrote:
> Hi,
>
> I do not understand the analysis and do not see that the spinlock is a
> problem here.
> I think your DSAC analyzer assumes that the FUNC_PTR mgr_ctrl call calls
> the  mgr_ctrl in tei.c, but in real it calls l2->ch.ctrl() which is the
> function in layer2.c, not tei.c. And the function in layer2.c should not
> do any GFP_KERNEL allocation.
>
> Same for your 2. reported issue.

Okay, thanks for your reply.
My analysis handles the function pointer using the function type and 
structure field, but it cannot distinguish the different variables of 
the same type and field now.
I will try to improve my tool, thanks for your explanation.


Best wishes,
Jia-Ju Bai