* Re: [PATCH net] net/iucv: take a reference on the socket found in afiucv_hs_rcv()
2026-07-06 3:24 [PATCH net] net/iucv: take a reference on the socket found in afiucv_hs_rcv() Bryam Vargas via B4 Relay
@ 2026-07-07 3:24 ` sashiko-bot
2026-07-08 16:06 ` Hidayathulla Khan I
2026-07-08 16:45 ` Hidayathulla Khan I
2 siblings, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-07-07 3:24 UTC (permalink / raw)
To: Bryam Vargas; +Cc: Alexander Gordeev, Heiko Carstens, Vasily Gorbik, linux-s390
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues:
- [High] NULL Pointer Dereference in `afiucv_hs_callback_syn` error path.
- [High] Shared SKB Data Corruption by in-place modification.
- [High] Buffer Underflow panic due to unchecked `skb_push()`.
- [High] Memory leak of `message_q.list` elements on socket destruction.
- [High] Race Condition modifying socket state from softirq without checking `sock_owned_by_user()`.
--
commit 65314896d356a76bf086aa52125b656e444bcb3c
Author: Bryam Vargas <hexlabsecurity@proton.me>
Subject: net/iucv: take a reference on the socket found in afiucv_hs_rcv()
afiucv_hs_rcv() looks up the destination socket under iucv_sk_list.lock, drops
the lock, and passes the socket to handlers without holding a reference. This
commit takes a reference with sock_hold() while the socket is on the list and
releases it with sock_put() once the handler completes.
> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index fed240b453bd9..b85fb9767dec2 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
[ ... ]
net/iucv/af_iucv.c:afiucv_hs_callback_syn() {
nsk = iucv_sock_alloc(NULL, GFP_ATOMIC, sk->sk_type, sk->sk_protocol);
if (!nsk) {
...
goto out;
}
...
out:
iucv_sock_kill(nsk);
}
[Severity: High]
This is a pre-existing issue, but does this error path cause a NULL pointer
dereference when memory allocation fails?
If iucv_sock_alloc() returns NULL under memory pressure, we jump to the out
label and pass NULL to iucv_sock_kill().
net/iucv/af_iucv.c:iucv_sock_kill() {
if (!sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket)
return;
...
}
Will iucv_sock_kill() immediately dereference the NULL sk pointer when checking
the socket flags, causing a kernel panic?
[ ... ]
net/iucv/af_iucv.c:afiucv_hs_rcv() {
...
trans_hdr = iucv_trans_hdr(skb);
EBCASC(trans_hdr->destAppName, sizeof(trans_hdr->destAppName));
EBCASC(trans_hdr->destUserID, sizeof(trans_hdr->destUserID));
...
}
[Severity: High]
This isn't a bug introduced by this patch, but does this in-place modification
corrupt shared or cloned SKBs?
Since afiucv_hs_rcv() acts as a packet_type handler, the received SKB might be
cloned or shared with other consumers. Should we be using skb_share_check() or
skb_unshare() before destructively modifying the header with EBCASC()?
Similarly, in afiucv_swap_src_dest():
net/iucv/af_iucv.c:afiucv_swap_src_dest() {
...
ASCEBC(trans_hdr->destUserID, sizeof(trans_hdr->destUserID));
...
}
Does this further modify the shared SKB payload in-place?
[ ... ]
net/iucv/af_iucv.c:afiucv_swap_src_dest() {
...
skb_push(skb, ETH_HLEN);
...
}
[Severity: High]
This is a pre-existing issue, but does this skb_push() call cause a buffer
underflow panic?
When echoing packets back, this prepends a MAC header without verifying if the
SKB has sufficient headroom.
If a packet is injected locally or the incoming interface strips the MAC header
tightly, will the lack of skb_headroom() checks result in skb_under_panic()?
[ ... ]
net/iucv/af_iucv.c:iucv_sock_destruct() {
...
skb_queue_purge(&sk->sk_receive_queue);
skb_queue_purge(&sk->sk_error_queue);
...
}
[Severity: High]
This isn't a bug introduced by this patch, but does socket destruction leak
the dynamically allocated message_q.list items?
Incoming messages are allocated and appended to iucv->message_q.list in
iucv_callback_rx().
If the socket is destroyed before all messages are consumed by recvmsg(),
neither iucv_sock_destruct() nor iucv_sock_close() appears to iterate over and
free these elements:
net/iucv/af_iucv.c:iucv_sock_close() {
...
skb_queue_purge(&iucv->send_skb_q);
skb_queue_purge(&iucv->backlog_skb_q);
...
}
Could an abrupt disconnect permanently leak these unread items?
[ ... ]
net/iucv/af_iucv.c:afiucv_hs_callback_synack() {
...
bh_lock_sock(sk);
iucv->msglimit_peer = iucv_trans_hdr(skb)->window;
sk->sk_state = IUCV_CONNECTED;
sk->sk_state_change(sk);
bh_unlock_sock(sk);
...
}
[Severity: High]
This is a pre-existing issue, but does modifying the socket state from a
softirq context corrupt the socket state machine?
Since this handler runs in softirq context when network packets arrive, it
acquires the socket spinlock and directly mutates sk->sk_state.
If a user process is concurrently executing a system call and holds the socket
mutex via lock_sock(), does bypassing the backlog mechanism by failing to check
sock_owned_by_user() cause race conditions?
[ ... ]
> @@ -2089,6 +2089,8 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
> }
> }
> }
> + if (sk)
> + sock_hold(sk);
> read_unlock(&iucv_sk_list.lock);
> if (!iucv)
> sk = NULL;
> @@ -2138,6 +2140,8 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
> kfree_skb(skb);
> }
>
> + if (sk)
> + sock_put(sk);
> return err;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260705-b4-disp-fc79c0dc-v1-1-d2cdcb57afa9@proton.me?part=1
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net] net/iucv: take a reference on the socket found in afiucv_hs_rcv()
2026-07-06 3:24 [PATCH net] net/iucv: take a reference on the socket found in afiucv_hs_rcv() Bryam Vargas via B4 Relay
2026-07-07 3:24 ` sashiko-bot
@ 2026-07-08 16:06 ` Hidayathulla Khan I
2026-07-08 16:45 ` Hidayathulla Khan I
2 siblings, 0 replies; 4+ messages in thread
From: Hidayathulla Khan I @ 2026-07-08 16:06 UTC (permalink / raw)
To: hexlabsecurity, Paolo Abeni, Jakub Kicinski, Eric Dumazet,
David S. Miller, Thorsten Winkler, Alexandra Winter
Cc: Nagamani PV, Simon Horman, netdev, linux-kernel, linux-s390,
pasic, aswin
Hi Bryam,
Addressing Sashiko's findings on this patch: [High] NULL Pointer Dereference in `afiucv_hs_callback_syn` error path.
This is already fixed by my patch currently under review.
[PATCH net] net/af_iucv: fix NULL deref in afiucv_hs_callback_syn()
The fix guards iucv_sock_kill(nsk) with if (nsk).
Regards,
Hidayath Khan
On 06/07/26 8:54 am, Bryam Vargas via B4 Relay wrote:
> From: Bryam Vargas <hexlabsecurity@proton.me>
>
> afiucv_hs_rcv() looks up the destination socket under iucv_sk_list.lock,
> drops the lock, and then passes the socket to the afiucv_hs_callback_*()
> handlers without holding a reference. AF_IUCV sockets are not
> RCU-protected and are freed synchronously by iucv_sock_kill() ->
> sock_put(), so a concurrent close can free the socket in the window
> between read_unlock() and the handler, which then dereferences freed
> memory (for example sk->sk_data_ready() in afiucv_hs_callback_syn()).
>
> Take a reference with sock_hold() while the socket is still on the list
> and release it with sock_put() once the handler has run.
>
> Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
> ---
> afiucv_netdev_event() has the same iucv_sk_list use-after-free and is
> being fixed separately by Nagamani PV:
> https://lore.kernel.org/all/20260508170534.2208812-1-nagamani@linux.ibm.com/
> This patch covers the receive path, afiucv_hs_rcv(), which that fix does
> not touch.
>
> Verified with an LKMM/herd7 litmus (the missing sock_hold() is the only
> delta between a flagged data race and none) and with an in-kernel KASAN
> model that reproduces the afiucv_hs_rcv() lookup-without-hold path (the
> freed socket's sk_data_ready pointer is read after the callback runs);
> adding the reference clears it. af_iucv is s390-only, so this is the
> model rather than the driver. Reproducer available on request.
> ---
> net/iucv/af_iucv.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index fed240b453bd..b85fb9767dec 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -2089,6 +2089,8 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
> }
> }
> }
> + if (sk)
> + sock_hold(sk);
> read_unlock(&iucv_sk_list.lock);
> if (!iucv)
> sk = NULL;
> @@ -2138,6 +2140,8 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
> kfree_skb(skb);
> }
>
> + if (sk)
> + sock_put(sk);
> return err;
> }
>
>
> ---
> base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
> change-id: 20260705-b4-disp-fc79c0dc-019670262472
>
> Best regards,
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net] net/iucv: take a reference on the socket found in afiucv_hs_rcv()
2026-07-06 3:24 [PATCH net] net/iucv: take a reference on the socket found in afiucv_hs_rcv() Bryam Vargas via B4 Relay
2026-07-07 3:24 ` sashiko-bot
2026-07-08 16:06 ` Hidayathulla Khan I
@ 2026-07-08 16:45 ` Hidayathulla Khan I
2 siblings, 0 replies; 4+ messages in thread
From: Hidayathulla Khan I @ 2026-07-08 16:45 UTC (permalink / raw)
To: hexlabsecurity, Paolo Abeni, Jakub Kicinski, Eric Dumazet,
David S. Miller, Thorsten Winkler, Alexandra Winter, pasic
Cc: Nagamani PV, Simon Horman, netdev, linux-kernel, linux-s390,
aswin
Hi Bryam,
Patch looks correct. The fix properly pins the socket found in
afiucv_hs_rcv()
before dropping iucv_sk_list.lock, preventing a concurrent close from
freeing
it before the handlers run.
The sock_hold() and sock_put() placement and guards are correct.
Reviewed-by: Hidayath Khan <hidayath@linux.ibm.com>
On 06/07/26 8:54 am, Bryam Vargas via B4 Relay wrote:
> From: Bryam Vargas <hexlabsecurity@proton.me>
>
> afiucv_hs_rcv() looks up the destination socket under iucv_sk_list.lock,
> drops the lock, and then passes the socket to the afiucv_hs_callback_*()
> handlers without holding a reference. AF_IUCV sockets are not
> RCU-protected and are freed synchronously by iucv_sock_kill() ->
> sock_put(), so a concurrent close can free the socket in the window
> between read_unlock() and the handler, which then dereferences freed
> memory (for example sk->sk_data_ready() in afiucv_hs_callback_syn()).
>
> Take a reference with sock_hold() while the socket is still on the list
> and release it with sock_put() once the handler has run.
>
> Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
> ---
> afiucv_netdev_event() has the same iucv_sk_list use-after-free and is
> being fixed separately by Nagamani PV:
> https://lore.kernel.org/all/20260508170534.2208812-1-nagamani@linux.ibm.com/
> This patch covers the receive path, afiucv_hs_rcv(), which that fix does
> not touch.
>
> Verified with an LKMM/herd7 litmus (the missing sock_hold() is the only
> delta between a flagged data race and none) and with an in-kernel KASAN
> model that reproduces the afiucv_hs_rcv() lookup-without-hold path (the
> freed socket's sk_data_ready pointer is read after the callback runs);
> adding the reference clears it. af_iucv is s390-only, so this is the
> model rather than the driver. Reproducer available on request.
> ---
> net/iucv/af_iucv.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index fed240b453bd..b85fb9767dec 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -2089,6 +2089,8 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
> }
> }
> }
> + if (sk)
> + sock_hold(sk);
> read_unlock(&iucv_sk_list.lock);
> if (!iucv)
> sk = NULL;
> @@ -2138,6 +2140,8 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
> kfree_skb(skb);
> }
>
> + if (sk)
> + sock_put(sk);
> return err;
> }
>
>
> ---
> base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
> change-id: 20260705-b4-disp-fc79c0dc-019670262472
>
> Best regards,
^ permalink raw reply [flat|nested] 4+ messages in thread