Re: [PATCH net] net/iucv: only deliver HiperSockets frames to HiperSockets sockets

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unchecked `skb_push()` in `afiucv_swap_src_dest()` causes kernel panic (`skb_under_panic`) due to insufficient headroom.
- [High] In-place modification of potentially shared or cloned SKB payload in `afiucv_hs_rcv()`.
- [High] Invalid transmission of a shared/cloned RX SKB directly via `dev_queue_xmit()`.
- [High] Missing network namespace validation allows cross-namespace packet injection and processing.
- [High] Unprotected list iteration in `afiucv_netdev_event` without acquiring `iucv_sk_list.lock`.
- [High] `afiucv_hs_callback_txnotify` modifies socket state without holding the socket lock.
--

--- Patch [1]: [PATCH net] net/iucv: only deliver HiperSockets frames to HiperSockets sockets ---
commit c7ea9342df754a7a4600b67180607cae05a3e24c
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/iucv: only deliver HiperSockets frames to HiperSockets sockets
    
afiucv_hs_rcv() selects a socket out of iucv_sk_list by the four name
fields alone, with no test on iucv->transport, so a frame arriving over
HiperSockets can be delivered to a socket bound to the classic z/VM IUCV
transport. iucv_sock_bind() makes that reachable rather than theoretical:
a bind to the local guest userid always takes the classic path, even on a
guest that also carries a HiperSockets device with the same identifier.

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index ea047bab65e7c..5fb6793b9a64a 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -2079,6 +2079,8 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
>  	sk = NULL;
>  	read_lock(&iucv_sk_list.lock);
>  	sk_for_each(sk, &iucv_sk_list.head) {
> +		if (iucv_sk(sk)->transport != AF_IUCV_TRANS_HIPER)
> +			continue;
>  		if (trans_hdr->flags == AF_IUCV_FLAG_SYN) {
>  			if ((!memcmp(&iucv_sk(sk)->src_name,
>  				     trans_hdr->destAppName, 8)) &&

[Severity: High]
This is a pre-existing issue, but I noticed afiucv_hs_rcv() modifies the SKB
in-place without checking if it is shared or cloned:

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

Does this corrupt the payload for other consumers if the networking core
delivered the SKB as shared or cloned?

[Severity: High]
This is a pre-existing issue, but I noticed afiucv_hs_rcv() does not check the
network namespace before processing the packet:

net/iucv/af_iucv.c:afiucv_hs_rcv() {
    static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
                             struct packet_type *pt, struct net_device *orig_dev)
    {
        ...
    }
}

Could an unprivileged user with CAP_NET_RAW in a container namespace craft and
send ETH_P_AF_IUCV packets on their local dummy or loopback device, bypassing
namespace isolation?

[Severity: Critical]
This is a pre-existing issue, but I noticed an unchecked skb_push() that could
cause a panic:

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

When afiucv_hs_callback_syn() bounces unmatched SYN packets, it passes the
newly received SKB to afiucv_swap_src_dest(). If an attacker injects a SYN
packet with less than 14 bytes of headroom, will this unconditionally push into
the SKB headroom and trigger skb_under_panic()?

[Severity: High]
This is a pre-existing issue, but does afiucv_hs_callback_syn() pass a received
RX SKB directly to the TX path?

net/iucv/af_iucv.c:afiucv_hs_callback_syn() {
    ...
    afiucv_swap_src_dest(skb);
    trans_hdr->flags = AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_FIN;
    err = dev_queue_xmit(skb);
    ...
}

If the SKB was delivered as shared or cloned, will modifying and transmitting
it trigger a WARN_ON(1) in __dev_queue_xmit() and violate buffer handoff
safety?

[Severity: High]
This is a pre-existing issue, but does afiucv_netdev_event() iterate over the
socket list without acquiring the read lock?

net/iucv/af_iucv.c:afiucv_netdev_event() {
    ...
    case NETDEV_GOING_DOWN:
        sk_for_each(sk, &iucv_sk_list.head) {
            iucv = iucv_sk(sk);
            ...
}

While this callback runs with the RTNL lock held, can concurrent socket creation
or destruction (which only hold the iucv_sk_list.lock) modify the list and
cause a use-after-free or infinite loop here?

[Severity: High]
This is a pre-existing issue, but does afiucv_hs_callback_txnotify() modify the
socket state locklessly?

net/iucv/af_iucv.c:afiucv_hs_callback_txnotify() {
    ...
    if (sk->sk_state == IUCV_CONNECTED) {
        sk->sk_state = IUCV_DISCONN;
        sk->sk_state_change(sk);
    }
    ...
}

Since this is called by the underlying driver outside of process context, does
modifying the state without holding bh_lock_sock(sk) create data races with
other socket operations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813-b4-disp-60433a46-v1-1-509e1200533e@proton.me?part=1