|
Takashi Iwai |
63b669 |
From 286228d382ba6320f04fa2e7c6fc8d4d92e428f4 Mon Sep 17 00:00:00 2001
|
|
Takashi Iwai |
63b669 |
From: Oleksij Rempel <o.rempel@pengutronix.de>
|
|
Takashi Iwai |
63b669 |
Date: Wed, 18 Dec 2019 09:39:02 +0100
|
|
Takashi Iwai |
63b669 |
Subject: [PATCH] can: can_create_echo_skb(): fix echo skb generation: always use skb_clone()
|
|
Takashi Iwai |
63b669 |
Git-commit: 286228d382ba6320f04fa2e7c6fc8d4d92e428f4
|
|
Takashi Iwai |
63b669 |
Patch-mainline: v5.10-rc3
|
|
Takashi Iwai |
63b669 |
References: git-fixes
|
|
Takashi Iwai |
63b669 |
|
|
Takashi Iwai |
63b669 |
All user space generated SKBs are owned by a socket (unless injected into the
|
|
Takashi Iwai |
63b669 |
key via AF_PACKET). If a socket is closed, all associated skbs will be cleaned
|
|
Takashi Iwai |
63b669 |
up.
|
|
Takashi Iwai |
63b669 |
|
|
Takashi Iwai |
63b669 |
This leads to a problem when a CAN driver calls can_put_echo_skb() on a
|
|
Takashi Iwai |
63b669 |
unshared SKB. If the socket is closed prior to the TX complete handler,
|
|
Takashi Iwai |
63b669 |
can_get_echo_skb() and the subsequent delivering of the echo SKB to all
|
|
Takashi Iwai |
63b669 |
registered callbacks, a SKB with a refcount of 0 is delivered.
|
|
Takashi Iwai |
63b669 |
|
|
Takashi Iwai |
63b669 |
To avoid the problem, in can_get_echo_skb() the original SKB is now always
|
|
Takashi Iwai |
63b669 |
cloned, regardless of shared SKB or not. If the process exists it can now
|
|
Takashi Iwai |
63b669 |
safely discard its SKBs, without disturbing the delivery of the echo SKB.
|
|
Takashi Iwai |
63b669 |
|
|
Takashi Iwai |
63b669 |
The problem shows up in the j1939 stack, when it clones the incoming skb, which
|
|
Takashi Iwai |
63b669 |
detects the already 0 refcount.
|
|
Takashi Iwai |
63b669 |
|
|
Takashi Iwai |
63b669 |
We can easily reproduce this with following example:
|
|
Takashi Iwai |
63b669 |
|
|
Takashi Iwai |
63b669 |
testj1939 -B -r can0: &
|
|
Takashi Iwai |
63b669 |
cansend can0 1823ff40#0123
|
|
Takashi Iwai |
63b669 |
|
|
Takashi Iwai |
63b669 |
Warning: CPU: 0 PID: 293 at lib/refcount.c:25 refcount_warn_saturate+0x108/0x174
|
|
Takashi Iwai |
63b669 |
Refcount_t: addition on 0; use-after-free.
|
|
Takashi Iwai |
63b669 |
Modules linked in: coda_vpu imx_vdoa videobuf2_vmalloc dw_hdmi_ahb_audio vcan
|
|
Takashi Iwai |
63b669 |
Cpu: 0 PID: 293 Comm: cansend Not tainted 5.5.0-rc6-00376-g9e20dcb7040d #1
|
|
Takashi Iwai |
63b669 |
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
|
|
Takashi Iwai |
63b669 |
Backtrace:
|
|
Takashi Iwai |
63b669 |
[<c010f570>] (dump_backtrace) from [<c010f90c>] (show_stack+0x20/0x24)
|
|
Takashi Iwai |
63b669 |
[<c010f8ec>] (show_stack) from [<c0c3e1a4>] (dump_stack+0x8c/0xa0)
|
|
Takashi Iwai |
63b669 |
[<c0c3e118>] (dump_stack) from [<c0127fec>] (__warn+0xe0/0x108)
|
|
Takashi Iwai |
63b669 |
[<c0127f0c>] (__warn) from [<c01283c8>] (warn_slowpath_fmt+0xa8/0xcc)
|
|
Takashi Iwai |
63b669 |
[<c0128324>] (warn_slowpath_fmt) from [<c0539c0c>] (refcount_warn_saturate+0x108/0x174)
|
|
Takashi Iwai |
63b669 |
[<c0539b04>] (refcount_warn_saturate) from [<c0ad2cac>] (j1939_can_recv+0x20c/0x210)
|
|
Takashi Iwai |
63b669 |
[<c0ad2aa0>] (j1939_can_recv) from [<c0ac9dc8>] (can_rcv_filter+0xb4/0x268)
|
|
Takashi Iwai |
63b669 |
[<c0ac9d14>] (can_rcv_filter) from [<c0aca2cc>] (can_receive+0xb0/0xe4)
|
|
Takashi Iwai |
63b669 |
[<c0aca21c>] (can_receive) from [<c0aca348>] (can_rcv+0x48/0x98)
|
|
Takashi Iwai |
63b669 |
[<c0aca300>] (can_rcv) from [<c09b1fdc>] (__netif_receive_skb_one_core+0x64/0x88)
|
|
Takashi Iwai |
63b669 |
[<c09b1f78>] (__netif_receive_skb_one_core) from [<c09b2070>] (__netif_receive_skb+0x38/0x94)
|
|
Takashi Iwai |
63b669 |
[<c09b2038>] (__netif_receive_skb) from [<c09b2130>] (netif_receive_skb_internal+0x64/0xf8)
|
|
Takashi Iwai |
63b669 |
[<c09b20cc>] (netif_receive_skb_internal) from [<c09b21f8>] (netif_receive_skb+0x34/0x19c)
|
|
Takashi Iwai |
63b669 |
[<c09b21c4>] (netif_receive_skb) from [<c0791278>] (can_rx_offload_napi_poll+0x58/0xb4)
|
|
Takashi Iwai |
63b669 |
|
|
Takashi Iwai |
63b669 |
Fixes: 0ae89beb283a ("can: add destructor for self generated skbs")
|
|
Takashi Iwai |
63b669 |
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
|
|
Takashi Iwai |
63b669 |
Link: http://lore.kernel.org/r/20200124132656.22156-1-o.rempel@pengutronix.de
|
|
Takashi Iwai |
63b669 |
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
|
|
Takashi Iwai |
63b669 |
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
|
|
Takashi Iwai |
63b669 |
Acked-by: Takashi Iwai <tiwai@suse.de>
|
|
Takashi Iwai |
63b669 |
|
|
Takashi Iwai |
63b669 |
---
|
|
Takashi Iwai |
63b669 |
include/linux/can/skb.h | 20 ++++++++------------
|
|
Takashi Iwai |
63b669 |
1 file changed, 8 insertions(+), 12 deletions(-)
|
|
Takashi Iwai |
63b669 |
|
|
Takashi Iwai |
63b669 |
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
|
|
Takashi Iwai |
63b669 |
index 900b9f4e0605..fc61cf4eff1c 100644
|
|
Takashi Iwai |
63b669 |
--- a/include/linux/can/skb.h
|
|
Takashi Iwai |
63b669 |
+++ b/include/linux/can/skb.h
|
|
Takashi Iwai |
63b669 |
@@ -61,21 +61,17 @@ static inline void can_skb_set_owner(struct sk_buff *skb, struct sock *sk)
|
|
Takashi Iwai |
63b669 |
*/
|
|
Takashi Iwai |
63b669 |
static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
|
|
Takashi Iwai |
63b669 |
{
|
|
Takashi Iwai |
63b669 |
- if (skb_shared(skb)) {
|
|
Takashi Iwai |
63b669 |
- struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
|
|
Takashi Iwai |
63b669 |
+ struct sk_buff *nskb;
|
|
Takashi Iwai |
63b669 |
|
|
Takashi Iwai |
63b669 |
- if (likely(nskb)) {
|
|
Takashi Iwai |
63b669 |
- can_skb_set_owner(nskb, skb->sk);
|
|
Takashi Iwai |
63b669 |
- consume_skb(skb);
|
|
Takashi Iwai |
63b669 |
- return nskb;
|
|
Takashi Iwai |
63b669 |
- } else {
|
|
Takashi Iwai |
63b669 |
- kfree_skb(skb);
|
|
Takashi Iwai |
63b669 |
- return NULL;
|
|
Takashi Iwai |
63b669 |
- }
|
|
Takashi Iwai |
63b669 |
+ nskb = skb_clone(skb, GFP_ATOMIC);
|
|
Takashi Iwai |
63b669 |
+ if (unlikely(!nskb)) {
|
|
Takashi Iwai |
63b669 |
+ kfree_skb(skb);
|
|
Takashi Iwai |
63b669 |
+ return NULL;
|
|
Takashi Iwai |
63b669 |
}
|
|
Takashi Iwai |
63b669 |
|
|
Takashi Iwai |
63b669 |
- /* we can assume to have an unshared skb with proper owner */
|
|
Takashi Iwai |
63b669 |
- return skb;
|
|
Takashi Iwai |
63b669 |
+ can_skb_set_owner(nskb, skb->sk);
|
|
Takashi Iwai |
63b669 |
+ consume_skb(skb);
|
|
Takashi Iwai |
63b669 |
+ return nskb;
|
|
Takashi Iwai |
63b669 |
}
|
|
Takashi Iwai |
63b669 |
|
|
Takashi Iwai |
63b669 |
#endif /* !_CAN_SKB_H */
|
|
Takashi Iwai |
63b669 |
--
|
|
Takashi Iwai |
63b669 |
2.16.4
|
|
Takashi Iwai |
63b669 |
|