| Message ID | 20240228163840.6667-1-pchelkin@ispras.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [wpan] mac802154: fix llsec key resources release in mac802154_llsec_key_del | expand |
Hi, On Wed, Feb 28, 2024 at 11:44 AM Fedor Pchelkin <pchelkin@ispras.ru> wrote: > > 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 <pchelkin@ispras.ru> > --- > 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? Thanks. Otherwise the patch looks correct to me. - Alex
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 <pchelkin@ispras.ru> wrote: > > > > 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 <pchelkin@ispras.ru> > > --- > > 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
Hi, On Mon, Mar 4, 2024 at 2:25 AM Fedor Pchelkin <pchelkin@ispras.ru> wrote: > > 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 <pchelkin@ispras.ru> wrote: > > > > > > 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 <pchelkin@ispras.ru> > > > --- > > > 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`? > Yes, I was confused about "ieee802154_llsec_key_entry" vs "ieee802154_llsec_key". Acked-by: Alexander Aring <aahringo@redhat.com> Thanks. - Alex
Hello. On 28.02.24 17:38, Fedor Pchelkin wrote: > 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 <pchelkin@ispras.ru> This patch has been applied to the wpan-next tree and will be part of the next pull request to net-next. Thanks! regards Stefan Schmidt
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); +} + int mac802154_llsec_key_del(struct mac802154_llsec *sec, const struct ieee802154_llsec_key_id *key) { struct ieee802154_llsec_key_entry *pos; list_for_each_entry(pos, &sec->table.keys, list) { - struct mac802154_llsec_key *mkey; - - mkey = container_of(pos->key, struct mac802154_llsec_key, key); - if (llsec_key_id_equal(&pos->id, key)) { list_del_rcu(&pos->list); - llsec_key_put(mkey); + call_rcu(&pos->rcu, mac802154_llsec_key_del_rcu); return 0; } }
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 <pchelkin@ispras.ru> --- 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(-)