git.ipfire.org Git - thirdparty/kernel/stable-queue.git/blob

   1 From f3510eec712bf6c9cb2d1ddeeb9886fc7b4584d0 Mon Sep 17 00:00:00 2001
   2 From: Sasha Levin <sashal@kernel.org>
   3 Date: Wed, 18 Dec 2019 09:39:02 +0100
   4 Subject: can: can_create_echo_skb(): fix echo skb generation: always use
   5  skb_clone()
   6
   7 From: Oleksij Rempel <o.rempel@pengutronix.de>
   8
   9 [ Upstream commit 286228d382ba6320f04fa2e7c6fc8d4d92e428f4 ]
  10
  11 All user space generated SKBs are owned by a socket (unless injected into the
  12 key via AF_PACKET). If a socket is closed, all associated skbs will be cleaned
  13 up.
  14
  15 This leads to a problem when a CAN driver calls can_put_echo_skb() on a
  16 unshared SKB. If the socket is closed prior to the TX complete handler,
  17 can_get_echo_skb() and the subsequent delivering of the echo SKB to all
  18 registered callbacks, a SKB with a refcount of 0 is delivered.
  19
  20 To avoid the problem, in can_get_echo_skb() the original SKB is now always
  21 cloned, regardless of shared SKB or not. If the process exists it can now
  22 safely discard its SKBs, without disturbing the delivery of the echo SKB.
  23
  24 The problem shows up in the j1939 stack, when it clones the incoming skb, which
  25 detects the already 0 refcount.
  26
  27 We can easily reproduce this with following example:
  28
  29 testj1939 -B -r can0: &
  30 cansend can0 1823ff40#0123
  31
  32 WARNING: CPU: 0 PID: 293 at lib/refcount.c:25 refcount_warn_saturate+0x108/0x174
  33 refcount_t: addition on 0; use-after-free.
  34 Modules linked in: coda_vpu imx_vdoa videobuf2_vmalloc dw_hdmi_ahb_audio vcan
  35 CPU: 0 PID: 293 Comm: cansend Not tainted 5.5.0-rc6-00376-g9e20dcb7040d #1
  36 Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
  37 Backtrace:
  38 [<c010f570>] (dump_backtrace) from [<c010f90c>] (show_stack+0x20/0x24)
  39 [<c010f8ec>] (show_stack) from [<c0c3e1a4>] (dump_stack+0x8c/0xa0)
  40 [<c0c3e118>] (dump_stack) from [<c0127fec>] (__warn+0xe0/0x108)
  41 [<c0127f0c>] (__warn) from [<c01283c8>] (warn_slowpath_fmt+0xa8/0xcc)
  42 [<c0128324>] (warn_slowpath_fmt) from [<c0539c0c>] (refcount_warn_saturate+0x108/0x174)
  43 [<c0539b04>] (refcount_warn_saturate) from [<c0ad2cac>] (j1939_can_recv+0x20c/0x210)
  44 [<c0ad2aa0>] (j1939_can_recv) from [<c0ac9dc8>] (can_rcv_filter+0xb4/0x268)
  45 [<c0ac9d14>] (can_rcv_filter) from [<c0aca2cc>] (can_receive+0xb0/0xe4)
  46 [<c0aca21c>] (can_receive) from [<c0aca348>] (can_rcv+0x48/0x98)
  47 [<c0aca300>] (can_rcv) from [<c09b1fdc>] (__netif_receive_skb_one_core+0x64/0x88)
  48 [<c09b1f78>] (__netif_receive_skb_one_core) from [<c09b2070>] (__netif_receive_skb+0x38/0x94)
  49 [<c09b2038>] (__netif_receive_skb) from [<c09b2130>] (netif_receive_skb_internal+0x64/0xf8)
  50 [<c09b20cc>] (netif_receive_skb_internal) from [<c09b21f8>] (netif_receive_skb+0x34/0x19c)
  51 [<c09b21c4>] (netif_receive_skb) from [<c0791278>] (can_rx_offload_napi_poll+0x58/0xb4)
  52
  53 Fixes: 0ae89beb283a ("can: add destructor for self generated skbs")
  54 Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
  55 Link: http://lore.kernel.org/r/20200124132656.22156-1-o.rempel@pengutronix.de
  56 Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
  57 Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
  58 Signed-off-by: Sasha Levin <sashal@kernel.org>
  59 ---
  60  include/linux/can/skb.h | 20 ++++++++------------
  61  1 file changed, 8 insertions(+), 12 deletions(-)
  62
  63 diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
  64 index b3379a97245c1..a34694e675c9a 100644
  65 --- a/include/linux/can/skb.h
  66 +++ b/include/linux/can/skb.h
  67 @@ -61,21 +61,17 @@ static inline void can_skb_set_owner(struct sk_buff *skb, struct sock *sk)
  68   */
  69  static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
  70  {
  71 -       if (skb_shared(skb)) {
  72 -               struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
  73 +       struct sk_buff *nskb;
  74
  75 -               if (likely(nskb)) {
  76 -                       can_skb_set_owner(nskb, skb->sk);
  77 -                       consume_skb(skb);
  78 -                       return nskb;
  79 -               } else {
  80 -                       kfree_skb(skb);
  81 -                       return NULL;
  82 -               }
  83 +       nskb = skb_clone(skb, GFP_ATOMIC);
  84 +       if (unlikely(!nskb)) {
  85 +               kfree_skb(skb);
  86 +               return NULL;
  87         }
  88
  89 -       /* we can assume to have an unshared skb with proper owner */
  90 -       return skb;
  91 +       can_skb_set_owner(nskb, skb->sk);
  92 +       consume_skb(skb);
  93 +       return nskb;
  94  }
  95
  96  #endif /* !_CAN_SKB_H */
  97 --
  98 2.27.0
  99