Re: [PATCH net-next] net/iucv: fix UAF in afiucv_netdev_event()

On 03.08.26 20:20, Nagamani PV wrote:
> afiucv_netdev_event() traverses iucv_sk_list without holding
> iucv_sk_list.lock.
> 
> A concurrent socket teardown can unlink and free an af_iucv socket via
> iucv_sock_kill() while the netdevice notifier path is still traversing
> the list, resulting in a use-after-free when dereferencing the socket.
> 
> syzbot reported a KASAN slab-use-after-free triggered from the netdevice
> notifier path:
> 
>   BUG: KASAN: slab-use-after-free in afiucv_netdev_event+0x166/0x570
>   Read of size 8 at addr 001a54009dcda368 by task syz.3.24/516
> 
>   Call Trace:
>     afiucv_netdev_event+0x166/0x570 net/iucv/af_iucv.c:2193
>     notifier_call_chain+0x18e/0x510 kernel/notifier.c:85
>     call_netdevice_notifiers net/core/dev.c:2301 [inline]
>     __dev_close_many+0x1c6/0x680 net/core/dev.c:1747
>     unregister_netdevice_many_notify+0x9b6/0x2650 net/core/dev.c:12388
>     unregister_netdevice_queue+0x392/0x3d0 net/core/dev.c:12291
>     __tun_detach+0xa50/0x1990 drivers/net/tun.c:621
> 
>   Allocated by task 521:
>     sk_prot_alloc+0xd2/0x230 net/core/sock.c:2247
>     sk_alloc+0x48/0x680 net/core/sock.c:2303
>     iucv_sock_alloc+0x4e/0x710 net/iucv/af_iucv.c:456
>     iucv_sock_create+0x114/0x180 net/iucv/af_iucv.c:2254
> 
>   Freed by task 519:
>     kfree+0x164/0x500 mm/slub.c:6561
>     iucv_sock_release+0x12e/0x150 net/iucv/af_iucv.c:1482
>     sock_close+0xa4/0x230 net/socket.c:1514
> 
> Protect the list traversal with the existing read-side lock.
> Use read_lock_bh()/read_unlock_bh() to synchronize with
> write_lock_bh()-protected updates to iucv_sk_list.
> 
> Fixes: 9fbd87d41392 ("af_iucv: handle netdev events")
> Cc: [email protected]
> Suggested-by: Hidayath Khan <[email protected]>
> Signed-off-by: Nagamani PV <[email protected]>
> 
> ---
>  net/iucv/af_iucv.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index ea047bab65e7..034039c50941 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -2213,6 +2213,7 @@ static int afiucv_netdev_event(struct notifier_block *this,
>  	switch (event) {
>  	case NETDEV_REBOOT:
>  	case NETDEV_GOING_DOWN:
> +		read_lock_bh(&iucv_sk_list.lock);
>  		sk_for_each(sk, &iucv_sk_list.head) {
>  			iucv = iucv_sk(sk);
>  			if ((iucv->hs_dev == event_dev) &&
> @@ -2223,6 +2224,7 @@ static int afiucv_netdev_event(struct notifier_block *this,
>  				sk->sk_state_change(sk);
>  			}
>  		}
> +		read_unlock_bh(&iucv_sk_list.lock);
>  		break;
>  	case NETDEV_DOWN:
>  	case NETDEV_UNREGISTER:

Nagmani,
As this is a fix, it should have been prefixed with net and not net-next.


See the discussions with Bryam Vargas [1]:
It is correct that afiucv_netdev_event() is missing lock protection.

However for a complete solution it should call lock_sock()
(because netdev events are called in process context) and handle
owned_by_user and a backlog queue.

So this patch improves the situation, but is not the complete solution.
I'd rather continue to work on a more complete fix than take this one,
but both ways are possible.

Kind regards
Alexandra

[1] https://lore.kernel.org/netdev/[email protected]/