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

   1 From e940e0895a82c6fbaa259f2615eb52b57ee91a7e Mon Sep 17 00:00:00 2001
   2 From: Oleksij Rempel <o.rempel@pengutronix.de>
   3 Date: Fri, 26 Feb 2021 10:24:56 +0100
   4 Subject: can: skb: can_skb_set_owner(): fix ref counting if socket was closed before setting skb ownership
   5
   6 From: Oleksij Rempel <o.rempel@pengutronix.de>
   7
   8 commit e940e0895a82c6fbaa259f2615eb52b57ee91a7e upstream.
   9
  10 There are two ref count variables controlling the free()ing of a socket:
  11 - struct sock::sk_refcnt - which is changed by sock_hold()/sock_put()
  12 - struct sock::sk_wmem_alloc - which accounts the memory allocated by
  13   the skbs in the send path.
  14
  15 In case there are still TX skbs on the fly and the socket() is closed,
  16 the struct sock::sk_refcnt reaches 0. In the TX-path the CAN stack
  17 clones an "echo" skb, calls sock_hold() on the original socket and
  18 references it. This produces the following back trace:
  19
  20 | WARNING: CPU: 0 PID: 280 at lib/refcount.c:25 refcount_warn_saturate+0x114/0x134
  21 | refcount_t: addition on 0; use-after-free.
  22 | Modules linked in: coda_vpu(E) v4l2_jpeg(E) videobuf2_vmalloc(E) imx_vdoa(E)
  23 | CPU: 0 PID: 280 Comm: test_can.sh Tainted: G            E     5.11.0-04577-gf8ff6603c617 #203
  24 | Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
  25 | Backtrace:
  26 | [<80bafea4>] (dump_backtrace) from [<80bb0280>] (show_stack+0x20/0x24) r7:00000000 r6:600f0113 r5:00000000 r4:81441220
  27 | [<80bb0260>] (show_stack) from [<80bb593c>] (dump_stack+0xa0/0xc8)
  28 | [<80bb589c>] (dump_stack) from [<8012b268>] (__warn+0xd4/0x114) r9:00000019 r8:80f4a8c2 r7:83e4150c r6:00000000 r5:00000009 r4:80528f90
  29 | [<8012b194>] (__warn) from [<80bb09c4>] (warn_slowpath_fmt+0x88/0xc8) r9:83f26400 r8:80f4a8d1 r7:00000009 r6:80528f90 r5:00000019 r4:80f4a8c2
  30 | [<80bb0940>] (warn_slowpath_fmt) from [<80528f90>] (refcount_warn_saturate+0x114/0x134) r8:00000000 r7:00000000 r6:82b44000 r5:834e5600 r4:83f4d540
  31 | [<80528e7c>] (refcount_warn_saturate) from [<8079a4c8>] (__refcount_add.constprop.0+0x4c/0x50)
  32 | [<8079a47c>] (__refcount_add.constprop.0) from [<8079a57c>] (can_put_echo_skb+0xb0/0x13c)
  33 | [<8079a4cc>] (can_put_echo_skb) from [<8079ba98>] (flexcan_start_xmit+0x1c4/0x230) r9:00000010 r8:83f48610 r7:0fdc0000 r6:0c080000 r5:82b44000 r4:834e5600
  34 | [<8079b8d4>] (flexcan_start_xmit) from [<80969078>] (netdev_start_xmit+0x44/0x70) r9:814c0ba0 r8:80c8790c r7:00000000 r6:834e5600 r5:82b44000 r4:82ab1f00
  35 | [<80969034>] (netdev_start_xmit) from [<809725a4>] (dev_hard_start_xmit+0x19c/0x318) r9:814c0ba0 r8:00000000 r7:82ab1f00 r6:82b44000 r5:00000000 r4:834e5600
  36 | [<80972408>] (dev_hard_start_xmit) from [<809c6584>] (sch_direct_xmit+0xcc/0x264) r10:834e5600 r9:00000000 r8:00000000 r7:82b44000 r6:82ab1f00 r5:834e5600 r4:83f27400
  37 | [<809c64b8>] (sch_direct_xmit) from [<809c6c0c>] (__qdisc_run+0x4f0/0x534)
  38
  39 To fix this problem, only set skb ownership to sockets which have still
  40 a ref count > 0.
  41
  42 Fixes: 0ae89beb283a ("can: add destructor for self generated skbs")
  43 Cc: Oliver Hartkopp <socketcan@hartkopp.net>
  44 Cc: Andre Naujoks <nautsch2@gmail.com>
  45 Link: https://lore.kernel.org/r/20210226092456.27126-1-o.rempel@pengutronix.de
  46 Suggested-by: Eric Dumazet <edumazet@google.com>
  47 Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
  48 Reviewed-by: Oliver Hartkopp <socketcan@hartkopp.net>
  49 Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
  50 Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  51 ---
  52  include/linux/can/skb.h |    8 ++++++--
  53  1 file changed, 6 insertions(+), 2 deletions(-)
  54
  55 --- a/include/linux/can/skb.h
  56 +++ b/include/linux/can/skb.h
  57 @@ -49,8 +49,12 @@ static inline void can_skb_reserve(struc
  58
  59  static inline void can_skb_set_owner(struct sk_buff *skb, struct sock *sk)
  60  {
  61 -       if (sk) {
  62 -               sock_hold(sk);
  63 +       /* If the socket has already been closed by user space, the
  64 +        * refcount may already be 0 (and the socket will be freed
  65 +        * after the last TX skb has been freed). So only increase
  66 +        * socket refcount if the refcount is > 0.
  67 +        */
  68 +       if (sk && refcount_inc_not_zero(&sk->sk_refcnt)) {
  69                 skb->destructor = sock_efree;
  70                 skb->sk = sk;
  71         }