Re: [PATCH net] net/iucv: filter frames in afiucv_hs_rcv() by ingress device

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

Pre-existing issues:
- [High] In-place modification of a potentially shared or cloned SKB without unsharing corrupts memory for concurrent readers.
- [High] Blind reuse of RX SKB for TX without verifying headroom causes skb_under_panic (DoS) and malformed frames.
- [High] Unprotected list traversal in afiucv_netdev_event() risks list corruption and use-after-free.
- [High] Socket and netdev reference leak during incoming connection errors causes unbounded memory consumption and device unregister hangs.
- [High] AF_IUCV RX handlers modify socket state from SoftIRQ context without checking if the socket is currently owned by the user.
- [High] Unprivileged processes in restricted network namespaces can create AF_IUCV sockets bound to host interfaces, bypassing network isolation.
--

--- Patch [1]: [PATCH net] net/iucv: filter frames in afiucv_hs_rcv() by ingress device ---
commit c0a4d18765a00aae7fc517041e61031313bfee8c
Author: Alexandra Winter <[email protected]>

net/iucv: filter frames in afiucv_hs_rcv() by ingress device

This commit fixes several issues in afiucv_hs_rcv() where frames arriving
on any net_device could be delivered to an AF_IUCV socket purely based on
transport header fields. It resolves problems with delivering HiperSockets
frames to classic z/VM IUCV sockets, cross-device injection, and network
namespace isolation bypass by strictly checking the ingress device against
the socket's bound device.

Link: https://lore.kernel.org/netdev/[email protected]/ [1]
Link: https://lore.kernel.org/netdev/[email protected]/ [2]

> Third, all AF_IUCV sockets live in init_net, as iucv_sock_alloc() calls
> sk_alloc(&init_net, ...). But even frames arriving on netdev devices in a
> namespace can be delivered to an IUCV socket. So a process in an
> unprivileged user and network namespace holding only the CAP_NET_RAW
> capability valid within that namespace can send a raw ETH_P_AF_IUCV frame
> on its own lo device and have it matched against init_net sockets.

[Severity: High]
This is a pre-existing issue, but does creating the socket directly in
init_net without validating the caller's namespace allow unprivileged
processes to escape isolation?

In iucv_sock_create(), the socket is allocated unconditionally in init_net:

sk = iucv_sock_alloc(sock, protocol, GFP_KERNEL, kern);

Then in iucv_sock_bind(), it scans and allows binding to devices in
init_net regardless of the caller's actual namespace:

for_each_netdev_rcu(&init_net, dev) {

Could this allow an unprivileged container to create an AF_IUCV socket
and bind it to a physical host device?

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index ea047bab65e7c..4e5cc9da6e068 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)->hs_dev != dev)
> +			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 does afiucv_hs_rcv() modify a potentially
shared or cloned SKB in-place without unsharing it?

Earlier in afiucv_hs_rcv(), the payload is modified directly:

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

Since this SKB is received from packet_type delivery, could modifying it
without calling skb_unshare() or skb_cow() corrupt data for concurrent
readers like packet sniffers?

Furthermore, afiucv_swap_src_dest() also prepends data using skb_push()
and memset() on this same SKB.

[Severity: High]
This is another pre-existing issue, but is there a risk of an
skb_under_panic when rejecting a connection?

When an incoming SYN frame is rejected, the unmodified RX SKB is passed
to afiucv_swap_src_dest() via afiucv_hs_callback_syn():

net/iucv/af_iucv.c:afiucv_swap_src_dest() {
    ...
    skb_push(skb, ETH_HLEN);
    ...
}

If the incoming frame was crafted with zero or limited headroom, could
this skb_push() without checking skb_headroom() cause a kernel panic?
The SKB is then handed directly to dev_queue_xmit() without further
scrubbing.

[Severity: High]
This isn't a bug introduced by this patch, but does afiucv_netdev_event()
traverse the socket list safely?

When a netdev event occurs, the list is traversed without a lock:

net/iucv/af_iucv.c:afiucv_netdev_event() {
    ...
    sk_for_each(sk, &iucv_sk_list.head) {
    ...
}

Since other threads can concurrently modify this list during socket
creation or destruction, could this unprotected traversal lead to a
use-after-free or list corruption?

[Severity: High]
This is a pre-existing issue, but does rejecting an incoming connection
result in a socket and netdev reference leak?

If afiucv_hs_callback_syn() rejects a connection, it attempts to clean up:

net/iucv/af_iucv.c:afiucv_hs_callback_syn() {
    ...
    if (nsk)
        iucv_sock_kill(nsk);
    ...
}

However, the new socket was initialized with the SOCK_ZAPPED flag cleared,
which causes iucv_sock_kill() to exit early:

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

Could this early return skip unlinking the socket and dropping references,
leading to unbounded memory consumption and preventing device
unregistration?

[Severity: High]
This is a pre-existing issue, but do the AF_IUCV RX handlers safely
mutate socket state from SoftIRQ context?

Callbacks like afiucv_hs_callback_synack() acquire the bottom-half lock
and change the state directly:

net/iucv/af_iucv.c:afiucv_hs_callback_synack() {
    ...
    bh_lock_sock(sk);
    ...
    sk->sk_state = IUCV_CONNECTED;
    sk->sk_state_change(sk);
    bh_unlock_sock(sk);
    ...
}

If a user process is concurrently calling close() and holding lock_sock(),
could mutating the state here without checking sock_owned_by_user()
corrupt the state machine?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1