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

[Hidayathulla Khan I]

Thanks for the Sashiko AI review: On the findings it raised.

Finding 1: iucv_sock_kill() returns early unless SOCK_ZAPPED is set,
and the flag is never set on a freshly allocated child socket, so the
child socket and its pinned net_device leak on the error paths. I had 
already
spotted this leak (in afiucv_hs_callback_syn() and iucv_callback_connreq())
and Alexandra Winter and I are looking into it.

Both NULL deref and child sock leak come from the same root cause,
the child socket is allocated before the listen-state and accept-queue 
checks.
I will address them together in v2 by allocating the child socket only 
after the
listen-state and accept-queue checks, so the refused path has nothing to
release (no NULL to guard and no child socket to free).

And on the transmit-failure path release the already-constructed
child socket directly (dev_put, unlink, put the last reference) instead of
relying on iucv_sock_kill().

Finding 2: missing sock_hold on the afiucv_hs_rcv() lookup. Agreed.
Bryam Vargas has already submitted a patch for this.

The other findings look valid too. I will follow up on them separately.

Thanks,
Hidayath Khan

On 10/07/26 12:47 am, Hidayath Khan wrote:
> 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@xxxxxxxxxxxxxxx
> Reviewed-by: Alexandra Winter <wintera@xxxxxxxxxxxxx>
> Signed-off-by: Hidayath Khan <hidayath@xxxxxxxxxxxxx>
> ---
>   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