[PATCH net] net/af_iucv: fix NULL deref in afiucv_hs_callback_syn()

[PATCH net] net/af_iucv: fix NULL deref in afiucv_hs_callback_syn()

[Hidayath Khan]

afiucv_hs_callback_syn() allocates the child socket with GFP_ATOMIC.
If the allocation fails, nsk is NULL.

The connection-refused path is entered when the listen state check
fails, the accept backlog is full, or nsk is NULL. The code
unconditionally calls iucv_sock_kill(nsk) in that path.

iucv_sock_kill() does not accept a NULL socket pointer and immediately
dereferences sk via sock_flag(sk, SOCK_ZAPPED). When nsk is NULL,
calling iucv_sock_kill(nsk) results in a NULL pointer dereference.

Only call iucv_sock_kill() when a child socket was successfully
allocated.

Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
Cc: stable@vger.kernel.org
Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>
Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
---
 net/iucv/af_iucv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index fed240b453bd..f5b1ec44b6ae 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -1872,7 +1872,8 @@ static int afiucv_hs_callback_syn(struct sock *sk, struct sk_buff *skb)
 		afiucv_swap_src_dest(skb);
 		trans_hdr->flags = AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_FIN;
 		err = dev_queue_xmit(skb);
-		iucv_sock_kill(nsk);
+		if (nsk)
+			iucv_sock_kill(nsk);
 		bh_unlock_sock(sk);
 		goto out;
 	}

base-commit: 262b2eac463d880a664cf92af1107b4f9d84ad37
-- 
2.52.0

RE: [PATCH net] net/af_iucv: fix NULL deref in afiucv_hs_callback_syn()

[Jagielski, Jedrzej]

From: Hidayath Khan <hidayath@linux.ibm.com> 
Sent: Thursday, July 9, 2026 9:18 PM

>afiucv_hs_callback_syn() allocates the child socket with GFP_ATOMIC.
>If the allocation fails, nsk is NULL.
>
>The connection-refused path is entered when the listen state check
>fails, the accept backlog is full, or nsk is NULL. The code
>unconditionally calls iucv_sock_kill(nsk) in that path.
>
>iucv_sock_kill() does not accept a NULL socket pointer and immediately
>dereferences sk via sock_flag(sk, SOCK_ZAPPED). When nsk is NULL,
>calling iucv_sock_kill(nsk) results in a NULL pointer dereference.
>
>Only call iucv_sock_kill() when a child socket was successfully
>allocated.
>
>Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
>Cc: stable@vger.kernel.org
>Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>
>Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
>---
> net/iucv/af_iucv.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
>index fed240b453bd..f5b1ec44b6ae 100644
>--- a/net/iucv/af_iucv.c
>+++ b/net/iucv/af_iucv.c
>@@ -1872,7 +1872,8 @@ static int afiucv_hs_callback_syn(struct sock *sk, struct sk_buff *skb)
> 		afiucv_swap_src_dest(skb);
> 		trans_hdr->flags = AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_FIN;
> 		err = dev_queue_xmit(skb);
>-		iucv_sock_kill(nsk);
>+		if (nsk)

Hi Hidayath

why not to move this check into iucv_sock_kill()?
would prevent from potential similar issues in the future

>+			iucv_sock_kill(nsk);
> 		bh_unlock_sock(sk);
> 		goto out;
> 	}
>
>base-commit: 262b2eac463d880a664cf92af1107b4f9d84ad37
>-- 
>2.52.0

Re: [PATCH net] net/af_iucv: fix NULL deref in afiucv_hs_callback_syn()

[Hidayathulla Khan I]

On 10/07/26 3:04 pm, Jagielski, Jedrzej wrote:
> From: Hidayath Khan <hidayath@linux.ibm.com>
> Sent: Thursday, July 9, 2026 9:18 PM
>
>> afiucv_hs_callback_syn() allocates the child socket with GFP_ATOMIC.
>> If the allocation fails, nsk is NULL.
>>
>> The connection-refused path is entered when the listen state check
>> fails, the accept backlog is full, or nsk is NULL. The code
>> unconditionally calls iucv_sock_kill(nsk) in that path.
>>
>> iucv_sock_kill() does not accept a NULL socket pointer and immediately
>> dereferences sk via sock_flag(sk, SOCK_ZAPPED). When nsk is NULL,
>> calling iucv_sock_kill(nsk) results in a NULL pointer dereference.
>>
>> Only call iucv_sock_kill() when a child socket was successfully
>> allocated.
>>
>> Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
>> Cc: stable@vger.kernel.org
>> Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>
>> Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
>> ---
>> net/iucv/af_iucv.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
>> index fed240b453bd..f5b1ec44b6ae 100644
>> --- a/net/iucv/af_iucv.c
>> +++ b/net/iucv/af_iucv.c
>> @@ -1872,7 +1872,8 @@ static int afiucv_hs_callback_syn(struct sock *sk, struct sk_buff *skb)
>> 		afiucv_swap_src_dest(skb);
>> 		trans_hdr->flags = AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_FIN;
>> 		err = dev_queue_xmit(skb);
>> -		iucv_sock_kill(nsk);
>> +		if (nsk)
> Hi Hidayath
>
> why not to move this check into iucv_sock_kill()?
> would prevent from potential similar issues in the future
Hi Jedrzej,

Every other call to iucv_sock_kill() passes a non-NULL socket by 
construction.

If iucv_sock_kill() silently accepted NULL, a future caller wrongly
passing NULL would go unnoticed instead of being caught.

>
>> +			iucv_sock_kill(nsk);
>> 		bh_unlock_sock(sk);
>> 		goto out;
>> 	}
>>
>> base-commit: 262b2eac463d880a664cf92af1107b4f9d84ad37
>> -- 
>> 2.52.0
>
>

Re: [PATCH net] net/af_iucv: fix NULL deref in afiucv_hs_callback_syn()

[sashiko-bot]

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

Pre-existing issues:
- [High] Newly allocated sockets and their pinned net_devices are permanently leaked on error paths because iucv_sock_kill() ignores sockets without the SOCK_ZAPPED flag.
- [High] Missing socket reference count (sock_hold) when searching the global socket list leads to a Use-After-Free vulnerability.
- [High] In-place modification of potentially shared/cloned SKBs corrupts data for concurrent consumers, and missing headroom checks cause panics.
- [High] Unlocked traversal of the accept_q leads to list corruption and data races.
--

--- Patch [1]: [PATCH net] net/af_iucv: fix NULL deref in afiucv_hs_callback_syn() ---
commit df9d50569da59293e9b6bc6ce2e2ee48847f1ed8
Author: Hidayath Khan <hidayath@linux.ibm.com>

net/af_iucv: fix NULL deref in afiucv_hs_callback_syn()

afiucv_hs_callback_syn() allocates the child socket with GFP_ATOMIC.
If the allocation fails, nsk is NULL. The connection-refused path
unconditionally calls iucv_sock_kill(nsk), resulting in a NULL pointer
dereference, which this patch fixes by checking if nsk is valid.

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index fed240b453bd9..f5b1ec44b6ae3 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -1872,7 +1872,8 @@ static int afiucv_hs_callback_syn(struct sock *sk, struct sk_buff *skb)

[Severity: High]
This is a pre-existing issue, but is there a missing reference count when
looking up sockets?

In afiucv_hs_rcv(), the socket is found and the list lock is dropped without
acquiring a reference:

net/iucv/af_iucv.c:afiucv_hs_rcv() {
    ...
    read_unlock(&iucv_sk_list.lock);
    if (!iucv)
        sk = NULL;
    ...
    err = afiucv_hs_callback_syn(sk, skb);
}

Could another CPU concurrently close and destroy the socket, causing a
use-after-free when afiucv_hs_callback_syn() calls bh_lock_sock(sk)?

>  		afiucv_swap_src_dest(skb);

[Severity: High]
This isn't a bug introduced by this patch, but does afiucv_swap_src_dest()
safely modify the SKB?

The SKB payload is modified in-place, but there is no check to ensure
exclusive ownership:

net/iucv/af_iucv.c:afiucv_hs_rcv() {
    ...
    trans_hdr = iucv_trans_hdr(skb);
    EBCASC(trans_hdr->destAppName, sizeof(trans_hdr->destAppName));
    ...
}

Could this corrupt packet data for concurrent consumers if the SKB is shared
or cloned?

Additionally, afiucv_swap_src_dest() unconditionally pushes into the SKB
headroom:

net/iucv/af_iucv.c:afiucv_swap_src_dest() {
    ...
    skb_push(skb, ETH_HLEN);
    memset(skb->data, 0, ETH_HLEN);
}

If an untrusted packet is received with 0 headroom, will this cause an
skb_under_panic?

>  		trans_hdr->flags = AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_FIN;
>  		err = dev_queue_xmit(skb);
> -		iucv_sock_kill(nsk);
> +		if (nsk)
> +			iucv_sock_kill(nsk);

[Severity: High]
This is a pre-existing issue, but does iucv_sock_kill() actually clean up the
socket here?

When iucv_sock_alloc() creates the socket, the SOCK_ZAPPED flag is cleared.
However, iucv_sock_kill() immediately returns if SOCK_ZAPPED is not set:

net/iucv/af_iucv.c:iucv_sock_kill() {
    if (!sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket)
        return;
    ...
}

Does this mean newly allocated sockets and their pinned net_devices are
permanently leaked on this error path?

>  		bh_unlock_sock(sk);
>  		goto out;
>  	}

[Severity: High]
This is a pre-existing issue, but is the accept queue safely protected against
concurrent modifications?

In iucv_accept_dequeue(), the queue is traversed without holding
accept_q_lock:

net/iucv/af_iucv.c:iucv_accept_dequeue() {
    ...
    list_for_each_entry_safe(isk, n, &iucv_sk(parent)->accept_q, accept_q) {
    ...
}

Meanwhile, iucv_accept_enqueue() modifies the same list from softirq context
using the lock:

net/iucv/af_iucv.c:iucv_accept_enqueue() {
    ...
    spin_lock_irqsave(&par->accept_q_lock, flags);
    list_add_tail(&iucv_sk(sk)->accept_q, &par->accept_q);
    spin_unlock_irqrestore(&par->accept_q_lock, flags);
}

Since lock_sock(parent) does not block softirqs from acquiring bh_lock_sock,
can iucv_accept_enqueue() run concurrently and corrupt the list pointers?

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