2018-09-01 10:01:58

by Jia-Ju Bai

Subject: [PATCH] net: scm: Fix a possible sleep-in-atomic-context bug in scm_fp_copy()

The kernel module may sleep with holding a spinlock.

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

[FUNC] kmalloc(GFP_KERNEL)
net/core/scm.c, 85: kmalloc in scm_fp_copy
net/core/scm.c, 161: scm_fp_copy in __scm_send
./include/net/scm.h, 88: __scm_send in scm_send
net/unix/af_unix.c, 1600: scm_send in maybe_init_creds
net/unix/af_unix.c, 1983: maybe_init_creds in unix_stream_sendpage
net/unix/af_unix.c, 1973: spin_lock in unix_stream_sendpage

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 <[email protected]>
---
net/core/scm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/scm.c b/net/core/scm.c
index b1ff8a441748..49548f81c45b 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -82,7 +82,7 @@ static int scm_fp_copy(struct cmsghdr *cmsg, struct scm_fp_list **fplp)

if (!fpl)
{
- fpl = kmalloc(sizeof(struct scm_fp_list), GFP_KERNEL);
+ fpl = kmalloc(sizeof(struct scm_fp_list), GFP_ATOMIC);
if (!fpl)
return -ENOMEM;
*fplp = fpl;
--
2.17.0

2018-09-02 23:08:22

by David Miller

Subject: Re: [PATCH] net: scm: Fix a possible sleep-in-atomic-context bug in scm_fp_copy()

From: Jia-Ju Bai <[email protected]>
Date: Sat, 1 Sep 2018 18:00:26 +0800

> The kernel module may sleep with holding a spinlock.
>
> The function call paths (from bottom to top) in Linux-4.16 are:
>
> [FUNC] kmalloc(GFP_KERNEL)
> net/core/scm.c, 85: kmalloc in scm_fp_copy
> net/core/scm.c, 161: scm_fp_copy in __scm_send
> ./include/net/scm.h, 88: __scm_send in scm_send
> net/unix/af_unix.c, 1600: scm_send in maybe_init_creds
> net/unix/af_unix.c, 1983: maybe_init_creds in unix_stream_sendpage
> net/unix/af_unix.c, 1973: spin_lock in unix_stream_sendpage

Please, do a full analysis of the code for these changes you are
submitting.

Read maybe_init_creds(), it sets msg.msg_controllen to zero.

struct msghdr msg = { .msg_controllen = 0 };

When that is zero, __scm__send() is never called.

static __inline__ int scm_send(struct socket *sock, struct msghdr *msg,
struct scm_cookie *scm, bool forcecreds)
{
...
if (msg->msg_controllen <= 0)
return 0;
return __scm_send(sock, msg, scm);

If this bug existed, sleeping in atomic warnings would be triggering
all the time and people would report that.

2018-09-03 01:46:09

by Jia-Ju Bai

Subject: Re: [PATCH] net: scm: Fix a possible sleep-in-atomic-context bug in scm_fp_copy()

Thanks for your reply.


On 2018/9/3 7:01, David Miller wrote:
> From: Jia-Ju Bai <[email protected]>
> Date: Sat, 1 Sep 2018 18:00:26 +0800
>
>> The kernel module may sleep with holding a spinlock.
>>
>> The function call paths (from bottom to top) in Linux-4.16 are:
>>
>> [FUNC] kmalloc(GFP_KERNEL)
>> net/core/scm.c, 85: kmalloc in scm_fp_copy
>> net/core/scm.c, 161: scm_fp_copy in __scm_send
>> ./include/net/scm.h, 88: __scm_send in scm_send
>> net/unix/af_unix.c, 1600: scm_send in maybe_init_creds
>> net/unix/af_unix.c, 1983: maybe_init_creds in unix_stream_sendpage
>> net/unix/af_unix.c, 1973: spin_lock in unix_stream_sendpage
> Please, do a full analysis of the code for these changes you are
> submitting.
>
> Read maybe_init_creds(), it sets msg.msg_controllen to zero.
>
> struct msghdr msg = { .msg_controllen = 0 };
>
> When that is zero, __scm__send() is never called.

Oh, I did not notice this, sorry...

> static __inline__ int scm_send(struct socket *sock, struct msghdr *msg,
> struct scm_cookie *scm, bool forcecreds)
> {
> ...
> if (msg->msg_controllen <= 0)
> return 0;
> return __scm_send(sock, msg, scm);
>
> If this bug existed, sleeping in atomic warnings would be triggering
> all the time and people would report that.

Sorry for this false positive.
I will check the code more carefully before submitting my patches.


Best wishes,
Jia-Ju Bai