* Re: [PATCH net v2] net/iucv: fix races in afiucv_netdev_event()
2026-09-17 7:17 ` [PATCH net v2] net/iucv: fix races in afiucv_netdev_event() Nagamani PV
@ 2026-09-17 14:01 ` Alexandra Winter
0 siblings, 0 replies; 2+ messages in thread
From: Alexandra Winter @ 2026-09-17 14:01 UTC (permalink / raw)
To: Nagamani PV, netdev
Cc: davem, kuba, pabeni, edumazet, andrew+netdev, twinkler, horms,
hca, gor, agordeev, borntraeger, svens, linux-s390, linux-kernel,
hexlabsecurity, hidayath, stable
On 17.09.26 09:17, Nagamani PV wrote:
> afiucv_netdev_event() walks iucv_sk_list.head inside a bare
> sk_for_each() with no lock held, while concurrent paths can modify
> or destroy sockets in that list:
>
> BUG-1 Use-After-Free
> iucv_sock_kill() calls iucv_sock_unlink() under write_lock_bh
> followed by sock_put() which may free the sk. If the notifier
> is mid-traversal when sock_put() runs, the stale sk pointer
> dereference at iucv = iucv_sk(sk) is a use-after-free.
> KASAN reports: slab-use-after-free in afiucv_netdev_event.
> Confirmed with KASAN on IBM Z.
>
> BUG-2 Locking correctness: iucv_send_ctrl() without lock_sock()
> iucv_send_ctrl() is called without holding lock_sock(sk). The
> notifier reads iucv->hs_dev without the socket lock, while
> iucv_sock_close() writes iucv->hs_dev = NULL under lock_sock().
> iucv_send_ctrl() also reads and writes sk->sk_shutdown as bare
> accesses, racing with iucv_sock_close() which holds lock_sock()
> for the same fields. KCSAN reports both races.
> Confirmed with KCSAN on IBM Z:
>
> write to iucv->hs_dev of 8 bytes by task N on cpu M:
> iucv_sock_close+0x196 [af_iucv] (under lock_sock)
>
> read to iucv->hs_dev by task N on cpu M:
> afiucv_netdev_event [af_iucv] (no lock held)
>
> BUG-3 sk_state data race
> sk->sk_state is written without lock_sock() in the notifier, racing
> with iucv_sock_close() writing the same field under lock_sock().
> KCSAN reports: data-race in afiucv_netdev_event / iucv_sock_close.
> Confirmed with KCSAN on IBM Z:
>
> write to sk->sk_state by task N on cpu M:
> iucv_sock_close+0x196 [af_iucv] (under lock_sock)
>
> read to sk->sk_state by task N on cpu M:
> afiucv_netdev_event+0xa6 [af_iucv] (no lock held)
>
> Fix with a two-pass algorithm:
>
> Pass 1 (read_lock_bh): walk iucv_sk_list, sock_hold() each
> matching socket, collect into a local list. The read lock
> prevents concurrent write_lock_bh in iucv_sock_link/unlink
> from modifying the list while we take references.
>
> Pass 2 (lock_sock per socket): for each collected socket,
> acquire lock_sock to serialise against iucv_sock_close(),
> check sk_state under the lock, call iucv_send_ctrl() and
> update sk_state safely, then release_sock() + sock_put().
>
> This eliminates all three races:
> - BUG-1: read_lock_bh prevents sk from being unlinked and freed
> while we hold a reference to it.
> - BUG-2: iucv_send_ctrl() is now called under lock_sock(), not
> racing with concurrent socket close.
> - BUG-3: sk_state is read and written under lock_sock(),
> serialising against iucv_sock_close().
>
> Pass 1 reads iucv_sk(sk)->hs_dev under read_lock_bh to filter
> sockets belonging to the affected device. iucv_sock_close() writes
> hs_dev = NULL under lock_sock(), which is orthogonal to read_lock_bh.
> Use READ_ONCE() for the Pass 1 read and WRITE_ONCE() for the
> iucv_sock_close() write to document the intentional concurrent access
> and suppress KCSAN false positives. The read is safe: read_lock_bh
> prevents the socket from being freed; if hs_dev is concurrently
> cleared to NULL it will not match event_dev (a valid pointer) so the
> socket is correctly skipped.
>
> Fixes: 9fbd87d41392 ("af_iucv: handle netdev events")
> Reported-by: Bryam Vargas <hexlabsecurity@proton.me>
> Link: https://lore.kernel.org/netdev/20260724222917.134769-1-hexlabsecurity@proton.me/
> Suggested-by: Hidayath Khan <hidayath@linux.ibm.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Nagamani PV <nagamani@linux.ibm.com>
> ---
> Changes since v1 (2-line read_lock_bh fix, posted 2026-08-03):
> - Redesign as two-pass algorithm to fix all races in one patch:
> Pass 1: read_lock_bh + sock_hold() to safely collect matching
> sockets; Pass 2: lock_sock() per socket to act on sk_state,
> call iucv_send_ctrl() and update sk_state under the lock.
> - Fixes Sashiko Finding 1 (New/High): sleep-in-atomic - V1 called
> iucv_send_ctrl() inside read_lock_bh(); Pass 2 runs after
> read_unlock_bh() so GFP_KERNEL allocation is safe.
> - Fixes Sashiko Finding 2 (Pre-existing/High): lockless manipulation
> of sk_state, sk_shutdown, sk_socket - all now under lock_sock().
> - Add WRITE_ONCE(iucv->hs_dev, NULL) in iucv_sock_close() and
> READ_ONCE(iucv_sk(sk)->hs_dev) in Pass 1 to document intentional
> concurrent access across orthogonal locks and suppress KCSAN.
> - All three bugs confirmed with KASAN + KCSAN on IBM Z with
> before/after TAP results.
> - Retarget from net-next to net (Fixes: + Cc: stable).
> - Update subject from "fix UAF" to "fix races" to reflect full scope.
>
> Bryam Vargas: your RFC identified the lockless socket manipulation in
> afiucv_netdev_event() as part of your 17-context analysis. I have
> included Reported-by for that attribution. Please let me know if you
> are happy with this, or prefer a different tag.
Nagamani,
I have told you before, that I want us to work on a correct usage of
of lock_sock() and bh_lock_sock() in af_iucv.c (stage 1 and 2 in Bryam's plan [1])
before fixing the callers (stage 3).
The races you are referring to belong to the group that needs fixing.
afiucv_netdev_event() needs a lock, as Bryam mentioned. The 2 pass approach looks
good to me in general. But without a proper implemenation of the lock_sock mechanism,
it is far from complete.
Please hold it until Stage 1 (for TRANS_HIPER) is implemented.
>
> net/iucv/af_iucv.c | 57 ++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 52 insertions(+), 5 deletions(-)
>
> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index ea047bab65e7..b197f9a254a7 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -320,6 +320,12 @@ static void iucv_sock_unlink(struct iucv_sock_list *l, struct sock *sk)
> write_unlock_bh(&l->lock);
> }
>
> +/* Used by afiucv_netdev_event() two-pass algorithm */
> +struct iucv_netdev_todo {
> + struct list_head list;
> + struct sock *sk;
> +};
> +
> /* Kill socket (only if zapped and orphaned) */
> static void iucv_sock_kill(struct sock *sk)
> {
> @@ -445,7 +451,7 @@ static void iucv_sock_close(struct sock *sk)
>
> if (iucv->hs_dev) {
> dev_put(iucv->hs_dev);
> - iucv->hs_dev = NULL;
> + WRITE_ONCE(iucv->hs_dev, NULL);
> sk->sk_bound_dev_if = 0;
> }
>
> @@ -2207,21 +2213,62 @@ static int afiucv_netdev_event(struct notifier_block *this,
> unsigned long event, void *ptr)
> {
> struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
> + struct iucv_netdev_todo *entry, *tmp;
> + LIST_HEAD(todo);
> struct sock *sk;
> - struct iucv_sock *iucv;
>
> switch (event) {
> case NETDEV_REBOOT:
> case NETDEV_GOING_DOWN:
> + /*
> + * Pass 1: collect matching sockets under read_lock_bh.
> + *
> + * read_lock_bh(&iucv_sk_list.lock) excludes concurrent
> + * write_lock_bh in iucv_sock_link/unlink, so sk cannot
> + * be removed from the list or freed while we walk it.
> + * sock_hold() pins the sk so it survives after we drop
> + * the lock.
> + *
> + * iucv_sock_close() writes hs_dev = NULL under lock_sock,
> + * which is orthogonal to read_lock_bh. READ_ONCE() documents
> + * the intentional concurrent access: if hs_dev is being
> + * cleared to NULL it will not equal event_dev (a valid
> + * pointer) so the socket is correctly skipped.
> + */
> + 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)) {
> + if (READ_ONCE(iucv_sk(sk)->hs_dev) != event_dev)
> + continue;
> + entry = kmalloc_obj(*entry, GFP_ATOMIC);
> + if (!entry)
> + continue;
> + sock_hold(sk);
> + entry->sk = sk;
> + list_add_tail(&entry->list, &todo);
> + }
> + read_unlock_bh(&iucv_sk_list.lock);
> + /*
> + * Pass 2: act on each socket under lock_sock.
> + *
> + * lock_sock() serialises against iucv_sock_close() and
> + * sock_orphan(), so sk_state and sk_socket are stable.
> + * iucv_send_ctrl() may call sock_alloc_send_skb(GFP_KERNEL)
> + * which requires non-atomic context -- satisfied here because
> + * we are no longer holding read_lock_bh.
> + */
> + list_for_each_entry_safe(entry, tmp, &todo, list) {
> + sk = entry->sk;
> + lock_sock(sk);
> + if (sk->sk_state == IUCV_CONNECTED) {
> if (event == NETDEV_GOING_DOWN)
> iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);
> sk->sk_state = IUCV_DISCONN;
> sk->sk_state_change(sk);
> }
> + release_sock(sk);
> + sock_put(sk);
> + list_del(&entry->list);
> + kfree(entry);
> }
> break;
> case NETDEV_DOWN:
^ permalink raw reply [flat|nested] 2+ messages in thread