Re: Re: [PATCH wpan] mac802154: fix llsec key resources release in mac802154_llsec_key_del

Hello Alexander,

Thanks for review!

On 24/03/03 06:19PM, Alexander Aring wrote:

Hi,

On Wed, Feb 28, 2024 at 11:44 AM Fedor Pchelkin [off-list ref] wrote:

quoted

mac802154_llsec_key_del() can free resources of a key directly without
following the RCU rules for waiting before the end of a grace period. This
may lead to use-after-free in case llsec_lookup_key() is traversing the
list of keys in parallel with a key deletion:

refcount_t: addition on 0; use-after-free.
WARNING: CPU: 4 PID: 16000 at lib/refcount.c:25 refcount_warn_saturate+0x162/0x2a0
Modules linked in:
CPU: 4 PID: 16000 Comm: wpan-ping Not tainted 6.7.0 #19
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
RIP: 0010:refcount_warn_saturate+0x162/0x2a0
Call Trace:
 <TASK>
 llsec_lookup_key.isra.0+0x890/0x9e0
 mac802154_llsec_encrypt+0x30c/0x9c0
 ieee802154_subif_start_xmit+0x24/0x1e0
 dev_hard_start_xmit+0x13e/0x690
 sch_direct_xmit+0x2ae/0xbc0
 __dev_queue_xmit+0x11dd/0x3c20
 dgram_sendmsg+0x90b/0xd60
 __sys_sendto+0x466/0x4c0
 __x64_sys_sendto+0xe0/0x1c0
 do_syscall_64+0x45/0xf0
 entry_SYSCALL_64_after_hwframe+0x6e/0x76

Also, ieee802154_llsec_key_entry structures are not freed by
mac802154_llsec_key_del():

unreferenced object 0xffff8880613b6980 (size 64):
  comm "iwpan", pid 2176, jiffies 4294761134 (age 60.475s)
  hex dump (first 32 bytes):
    78 0d 8f 18 80 88 ff ff 22 01 00 00 00 00 ad de  x.......".......
    00 00 00 00 00 00 00 00 03 00 cd ab 00 00 00 00  ................
  backtrace:
    [<ffffffff81dcfa62>] __kmem_cache_alloc_node+0x1e2/0x2d0
    [<ffffffff81c43865>] kmalloc_trace+0x25/0xc0
    [<ffffffff88968b09>] mac802154_llsec_key_add+0xac9/0xcf0
    [<ffffffff8896e41a>] ieee802154_add_llsec_key+0x5a/0x80
    [<ffffffff8892adc6>] nl802154_add_llsec_key+0x426/0x5b0
    [<ffffffff86ff293e>] genl_family_rcv_msg_doit+0x1fe/0x2f0
    [<ffffffff86ff46d1>] genl_rcv_msg+0x531/0x7d0
    [<ffffffff86fee7a9>] netlink_rcv_skb+0x169/0x440
    [<ffffffff86ff1d88>] genl_rcv+0x28/0x40
    [<ffffffff86fec15c>] netlink_unicast+0x53c/0x820
    [<ffffffff86fecd8b>] netlink_sendmsg+0x93b/0xe60
    [<ffffffff86b91b35>] ____sys_sendmsg+0xac5/0xca0
    [<ffffffff86b9c3dd>] ___sys_sendmsg+0x11d/0x1c0
    [<ffffffff86b9c65a>] __sys_sendmsg+0xfa/0x1d0
    [<ffffffff88eadbf5>] do_syscall_64+0x45/0xf0
    [<ffffffff890000ea>] entry_SYSCALL_64_after_hwframe+0x6e/0x76

Handle the proper resource release in the RCU callback function
mac802154_llsec_key_del_rcu().

Note that if llsec_lookup_key() finds a key, it gets a refcount via
llsec_key_get() and locally copies key id from key_entry (which is a
list element). So it's safe to call llsec_key_put() and free the list
entry after the RCU grace period elapses.

Found by Linux Verification Center (linuxtesting.org).

Fixes: 5d637d5aabd8 ("mac802154: add llsec structures and mutators")
Cc: stable@vger.kernel.org
Signed-off-by: Fedor Pchelkin <redacted>
---
Should the patch be targeted to "net" tree directly?

 include/net/cfg802154.h |  1 +
 net/mac802154/llsec.c   | 18 +++++++++++++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
index cd95711b12b8..76d2cd2e2b30 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h

@@ -401,6 +401,7 @@ struct ieee802154_llsec_key {

 struct ieee802154_llsec_key_entry {
        struct list_head list;
+       struct rcu_head rcu;

        struct ieee802154_llsec_key_id id;
        struct ieee802154_llsec_key *key;

diff --git a/net/mac802154/llsec.c b/net/mac802154/llsec.c
index 8d2eabc71bbe..f13b07ebfb98 100644
--- a/net/mac802154/llsec.c
+++ b/net/mac802154/llsec.c

@@ -265,19 +265,27 @@ int mac802154_llsec_key_add(struct mac802154_llsec *sec,
        return -ENOMEM;
 }

+static void mac802154_llsec_key_del_rcu(struct rcu_head *rcu)
+{
+       struct ieee802154_llsec_key_entry *pos;
+       struct mac802154_llsec_key *mkey;
+
+       pos = container_of(rcu, struct ieee802154_llsec_key_entry, rcu);
+       mkey = container_of(pos->key, struct mac802154_llsec_key, key);
+
+       llsec_key_put(mkey);
+       kfree_sensitive(pos);

I don't think this kfree is right, "struct ieee802154_llsec_key_entry"
is declared as "non pointer" in "struct mac802154_llsec_key". The
memory that is part of "struct ieee802154_llsec_key_entry" should be
freed when llsec_key_put(), llsec_key_release() hits.

Or is there something I am missing here?

`struct ieee802154_llsec_key_entry` is not included into any other
struct. It is a standalone entity describing an entry in the
`ieee802154_llsec_table.keys` list.

Maybe you are confusing it with `struct ieee802154_llsec_key`?

When mac802154_llsec_key_add() is called, `struct ieee802154_llsec_key_entry`
objects are allocated using kzalloc() and are linked into the list.

`struct mac802154_llsec_key` object is allocated only if it has not
been allocated yet for some other llsec_key_id, otherwise its refcount
is incremented. Its lifecycle is managed with llsec_key_{get|put}
primitives. A pointer to this object is passed into
`struct ieee802154_llsec_key_entry`.

So the only way to reach `struct ieee802154_llsec_key_entry` objects is
through the list they belong to and they should be freed when they are
unlinked from the list.

E.g. see mac802154_llsec_destroy() where for &sec->table.keys this
sequence of llsec_key_put() for mkey and kfree_sensitive() for list entry
is done.

Thanks.

Otherwise the patch looks correct to me.

- Alex

--
Fedor