Re: [PATCH net] net/iucv: take a reference on the socket found in afiucv_hs_rcv()

[sashiko-bot@kernel.org]

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