Re: [PATCH net] tipc: protect node reset trace dump with node lock

[Tung Quang Nguyen]

> Subject: [PATCH net] tipc: protect node reset trace dump with node lock

  >BUG: KASAN: slab-use-after-free in tipc_link_dump+0x10cb/0x16b0
  >Read of size 4 by task ksoftirqd/0/14
  >Call Trace:
  > tipc_link_dump+0x10cb/0x16b0
  > tipc_node_dump+0x4bb/0x740
  > trace_event_raw_event_tipc_node_class+0x258/0x360
  > tipc_node_reset_links+0x14d/0x1a0
  > tipc_rcv+0x13f5/0x3030
  > tipc_udp_recv+0x4e3/0x670
  > Allocated by task 0:
  > tipc_link_create+0x1e1/0x1020
  > tipc_node_check_dest+0x7d2/0x11a0
  > tipc_disc_rcv+0xdbf/0x1430
  > Freed by task 89:
  >  kfree+0x131/0x3c0
  >  tipc_node_link_down+0x267/0x4b0
  >  tipc_node_delete_links+0xec/0x160
  > bearer_disable+0x107/0x260

Please decode above stack trace (using linux/scripts/decode_stacktrace.sh).

> Take the node read lock around the trace event. This keeps link pointer
> loads and all dump dereferences serialized against link deletion while
> preserving the trace contents and reset flow.

> Fixes: eb18a510b5cd ("tipc: add trace_events for tipc node")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Chengfeng Ye <nicoyip.dev@xxxxxxxxx>
> ---
> net/tipc/node.c | 2 ++
> 1 file changed, 2 insertions(+)

> diff --git a/net/tipc/node.c b/net/tipc/node.c
> index 683a136e53ef..127848e8a644 100644
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -1333,7 +1333,9 @@ static void tipc_node_reset_links(struct tipc_node *n)

>       pr_warn("Resetting all links to %x\n", n->addr);

> +       tipc_node_read_lock(n);
>        trace_tipc_node_reset_links(n, true, " ");
> +       tipc_node_read_unlock(n);

It is not correct using read lock because trace_tipc_node_reset_links() accesses link's queues that tipc_rcv() might access concurrently.
Please test this:

diff --git a/net/tipc/node.c b/net/tipc/node.c
index 683a136e53ef..bd91378b7540 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -1333,7 +1333,9 @@ static void tipc_node_reset_links(struct tipc_node *n)

        pr_warn("Resetting all links to %x\n", n->addr);

+       tipc_node_write_lock(n);
        trace_tipc_node_reset_links(n, true, " ");
+       tipc_node_write_unlock_fast(n);
        for (i = 0; i < MAX_BEARERS; i++) {
                tipc_node_link_down(n, i, false);
        }

>        for (i = 0; i < MAX_BEARERS; i++) {
>               tipc_node_link_down(n, i, false);
>        }