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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The patch introduces a sleep-in-atomic-context bug by calling a sleepable memory allocation function while holding a spinlock (`read_lock_bh`) that disables bottom halves.

Pre-existing issues:
- [High] Socket state, shutdown flags, and transmissions are manipulated locklessly, leading to data races, lost state updates, and a potential NULL pointer dereference if the socket is orphaned concurrently.
--

--- Patch [1]: [PATCH net-next] net/iucv: fix UAF in afiucv_netdev_event() ---
commit 295060a75254f1d540b1b59d065d45ebee09c8e8
Author: Nagamani PV <nagamani@linux.ibm.com>
Subject: net/iucv: fix UAF in afiucv_netdev_event()

This commit aims to fix a KASAN slab-use-after-free in afiucv_netdev_event() by
protecting the socket list traversal with read_lock_bh() / read_unlock_bh().
This prevents concurrent socket teardown from freeing the socket during
iteration.

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index ea047bab65e7c..034039c509418 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) &&
>  			    (sk->sk_state == IUCV_CONNECTED)) {

[Severity: High]
This isn't a bug introduced by this patch, but does checking and modifying
the socket state here without taking the socket lock create a data race?

If a concurrent iucv_sock_release() sets the state to closed and calls
sock_orphan(sk) (which sets sk->sk_socket = NULL) before waiting on
iucv_sk_list.lock, could this event loop read the old state, get delayed,
and invoke iucv_send_ctrl() after the socket is orphaned?

When sock_alloc_send_pskb() is subsequently called inside iucv_send_ctrl(),
if the socket's write buffer is full, it tries to set SOCK_NOSPACE via
set_bit(SOCK_NOSPACE, &sk->sk_socket->flags). Would this result in a NULL
pointer dereference since sk->sk_socket was cleared?

The iucv_send_ctrl() function also directly manipulates sk->sk_shutdown
temporarily without locking the socket:

iucv_send_ctrl() {
    if (sk->sk_shutdown & SEND_SHUTDOWN) {
        shutdown = sk->sk_shutdown;
        sk->sk_shutdown &= RCV_SHUTDOWN;
    }
    ...
}

Does this create a race window that loses concurrent shutdown updates?

>  				if (event == NETDEV_GOING_DOWN)
>  					iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);

[Severity: High]
Does this introduce a sleep-in-atomic-context bug?

By acquiring read_lock_bh() earlier in afiucv_netdev_event(), the code now
executes in an atomic context. However, iucv_send_ctrl() performs a memory
allocation that can block:

afiucv_netdev_event()
   read_lock_bh(&iucv_sk_list.lock)
   iucv_send_ctrl()
      sock_alloc_send_skb()

The allocation relies on sk->sk_allocation (which defaults to
GFP_KERNEL | GFP_DMA for AF_IUCV sockets). Since these flags contain
__GFP_DIRECT_RECLAIM, will this allow the allocator to sleep and trigger
a "scheduling while atomic" panic?

>  				sk->sk_state = IUCV_DISCONN;
>  				sk->sk_state_change(sk);
>  			}
>  		}
> +		read_unlock_bh(&iucv_sk_list.lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803182053.2355882-1-nagamani@linux.ibm.com?part=1