* [PATCH] crypto: chelsio: chtls: fix possible sleep-in-atomic-context bugs in abort_syn_rcv()
@ 2019-12-18 3:34 Jia-Ju Bai
2019-12-27 2:57 ` Herbert Xu
0 siblings, 1 reply; 2+ messages in thread
From: Jia-Ju Bai @ 2019-12-18 3:34 UTC (permalink / raw)
To: atul.gupta, herbert, davem, tglx, allison, arjun, kstewart, edumazet
Cc: linux-crypto, linux-kernel, Jia-Ju Bai
The driver may sleep while holding a spinlock.
The function call path (from bottom to top) in Linux 4.19 is:
drivers/crypto/chelsio/chtls/chtls_cm.c, 1806:
alloc_skb(GFP_KERNEL) in send_abort_rpl
drivers/crypto/chelsio/chtls/chtls_cm.c, 1925:
send_abort_rpl in abort_syn_rcv
drivers/crypto/chelsio/chtls/chtls_cm.c, 1920:
spin_lock in abort_syn_rcv
drivers/crypto/chelsio/chtls/chtls_cm.c, 1787:
alloc_skb(GFP_KERNEL) in send_defer_abort_rpl
drivers/crypto/chelsio/chtls/chtls_cm.c, 1811:
send_defer_abort_rpl in send_abort_rpl
drivers/crypto/chelsio/chtls/chtls_cm.c, 1925:
send_abort_rpl in abort_syn_rcv
drivers/crypto/chelsio/chtls/chtls_cm.c, 1920:
spin_lock in abort_syn_rcv
alloc_skb(GFP_KERNEL) can sleep at runtime.
To fix these possible bugs, GFP_KERNEL is replaced with GFP_ATOMIC.
Besides, in send_defer_abort_rpl(), error handling code is added to
handle the failure of alloc_skb().
These bugs are found by a static analysis tool STCheck written by myself.
Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
drivers/crypto/chelsio/chtls/chtls_cm.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/crypto/chelsio/chtls/chtls_cm.c b/drivers/crypto/chelsio/chtls/chtls_cm.c
index aca75237bbcf..e6e4c3ddc368 100644
--- a/drivers/crypto/chelsio/chtls/chtls_cm.c
+++ b/drivers/crypto/chelsio/chtls/chtls_cm.c
@@ -1805,8 +1805,11 @@ static void send_defer_abort_rpl(struct chtls_dev *cdev, struct sk_buff *skb)
struct cpl_abort_req_rss *req = cplhdr(skb);
struct sk_buff *reply_skb;
- reply_skb = alloc_skb(sizeof(struct cpl_abort_rpl),
- GFP_KERNEL | __GFP_NOFAIL);
+ reply_skb = alloc_skb(sizeof(struct cpl_abort_rpl), GFP_ATOMIC);
+ if (!reply_skb) {
+ kfree_skb(skb);
+ return;
+ }
__skb_put(reply_skb, sizeof(struct cpl_abort_rpl));
set_abort_rpl_wr(reply_skb, GET_TID(req),
(req->status & CPL_ABORT_NO_RST));
@@ -1825,7 +1828,7 @@ static void send_abort_rpl(struct sock *sk, struct sk_buff *skb,
csk = rcu_dereference_sk_user_data(sk);
reply_skb = alloc_skb(sizeof(struct cpl_abort_rpl),
- GFP_KERNEL);
+ GFP_ATOMIC);
if (!reply_skb) {
req->status = (queue << 1);
--
2.17.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] crypto: chelsio: chtls: fix possible sleep-in-atomic-context bugs in abort_syn_rcv()
2019-12-18 3:34 [PATCH] crypto: chelsio: chtls: fix possible sleep-in-atomic-context bugs in abort_syn_rcv() Jia-Ju Bai
@ 2019-12-27 2:57 ` Herbert Xu
0 siblings, 0 replies; 2+ messages in thread
From: Herbert Xu @ 2019-12-27 2:57 UTC (permalink / raw)
To: Jia-Ju Bai
Cc: atul.gupta, davem, tglx, allison, arjun, kstewart, edumazet,
linux-crypto, linux-kernel
On Wed, Dec 18, 2019 at 11:34:22AM +0800, Jia-Ju Bai wrote:
> The driver may sleep while holding a spinlock.
> The function call path (from bottom to top) in Linux 4.19 is:
>
> drivers/crypto/chelsio/chtls/chtls_cm.c, 1806:
> alloc_skb(GFP_KERNEL) in send_abort_rpl
> drivers/crypto/chelsio/chtls/chtls_cm.c, 1925:
> send_abort_rpl in abort_syn_rcv
> drivers/crypto/chelsio/chtls/chtls_cm.c, 1920:
> spin_lock in abort_syn_rcv
>
> drivers/crypto/chelsio/chtls/chtls_cm.c, 1787:
> alloc_skb(GFP_KERNEL) in send_defer_abort_rpl
> drivers/crypto/chelsio/chtls/chtls_cm.c, 1811:
> send_defer_abort_rpl in send_abort_rpl
> drivers/crypto/chelsio/chtls/chtls_cm.c, 1925:
> send_abort_rpl in abort_syn_rcv
> drivers/crypto/chelsio/chtls/chtls_cm.c, 1920:
> spin_lock in abort_syn_rcv
>
> alloc_skb(GFP_KERNEL) can sleep at runtime.
>
> To fix these possible bugs, GFP_KERNEL is replaced with GFP_ATOMIC.
> Besides, in send_defer_abort_rpl(), error handling code is added to
> handle the failure of alloc_skb().
>
> These bugs are found by a static analysis tool STCheck written by myself.
>
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> ---
> drivers/crypto/chelsio/chtls/chtls_cm.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/crypto/chelsio/chtls/chtls_cm.c b/drivers/crypto/chelsio/chtls/chtls_cm.c
> index aca75237bbcf..e6e4c3ddc368 100644
> --- a/drivers/crypto/chelsio/chtls/chtls_cm.c
> +++ b/drivers/crypto/chelsio/chtls/chtls_cm.c
> @@ -1805,8 +1805,11 @@ static void send_defer_abort_rpl(struct chtls_dev *cdev, struct sk_buff *skb)
> struct cpl_abort_req_rss *req = cplhdr(skb);
> struct sk_buff *reply_skb;
>
> - reply_skb = alloc_skb(sizeof(struct cpl_abort_rpl),
> - GFP_KERNEL | __GFP_NOFAIL);
> + reply_skb = alloc_skb(sizeof(struct cpl_abort_rpl), GFP_ATOMIC);
> + if (!reply_skb) {
> + kfree_skb(skb);
> + return;
> + }
This code now makes no sense because it's just trying the same
kmalloc twice. Perhaps it really needs to be deferred as its
name suggests?
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-12-27 2:58 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-18 3:34 [PATCH] crypto: chelsio: chtls: fix possible sleep-in-atomic-context bugs in abort_syn_rcv() Jia-Ju Bai
2019-12-27 2:57 ` Herbert Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).