[PATCH net 0/2] net/iucv: give afiucv_hs_rcv() the preamble a packet_type handler needs

[PATCH net 0/2] net/iucv: give afiucv_hs_rcv() the preamble a packet_type handler needs

[Bryam Vargas via B4 Relay]

iucv_packet_type is registered with no .dev and no .af_packet_net, so
afiucv_hs_rcv() sits in the machine-global ptype_base[] and sees every frame of
its ethertype from every namespace, on any device. It then trusts the frame: no
namespace test, and no private or writable reference before it rewrites the
payload in place.

  1/2 drop frames from other namespaces -- the only one that crosses a
      privilege boundary, and the only one tagged for stable
  2/2 unshare and cow the head before the in-place rewrite

The two siblings differ in how far they go, so I will not claim more than they
do: net/x25/x25_dev.c does the namespace test and then takes a full skb_copy()
of every frame, and net/ieee802154/socket.c does the namespace test but does not
unshare at all. 2/2 takes the cheaper of the two -- skb_share_check() plus
skb_cow_head() -- which costs a refcount test and a compare when the frame is
already private, and copies only when it is not.

2/2 also covers the headroom that afiucv_swap_src_dest() pushes without
checking. That started as a third patch; it collapsed into this one once it was
clear skb_cow_head() is needed for writability anyway and asking it for ETH_HLEN
rather than nothing is free.

2/2 was surfaced by the sashiko.dev review bot on the thread of a previous af_iucv
patch, which flagged both the missing unshare and the unchecked push; the Closes:
trailer on that patch points at its report.

By inspection; not reproduced. Compile-tested for s390x.

---
Bryam Vargas (2):
      net/iucv: drop HiperSockets frames from other network namespaces
      net/iucv: take a private, writable frame before rewriting it in place

 net/iucv/af_iucv.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
---
base-commit: a59f57e2aa127c5354168d2ec4bac920df1be4f4
change-id: 20260815-b4-disp-dc82fde4-3cae5e52ae9f

Best regards,
--  
Bryam Vargas <hexlabsecurity@proton.me>

[PATCH net 0/2] net/iucv: give afiucv_hs_rcv() the preamble a packet_type handler needs

[Bryam Vargas]

iucv_packet_type is registered with no .dev and no .af_packet_net, so
afiucv_hs_rcv() sits in the machine-global ptype_base[] and sees every frame of
its ethertype from every namespace, on any device. It then trusts the frame: no
namespace test, and no private or writable reference before it rewrites the
payload in place.

  1/2 drop frames from other namespaces -- the only one that crosses a
      privilege boundary, and the only one tagged for stable
  2/2 unshare and cow the head before the in-place rewrite

The two siblings differ in how far they go, so I will not claim more than they
do: net/x25/x25_dev.c does the namespace test and then takes a full skb_copy()
of every frame, and net/ieee802154/socket.c does the namespace test but does not
unshare at all. 2/2 takes the cheaper of the two -- skb_share_check() plus
skb_cow_head() -- which costs a refcount test and a compare when the frame is
already private, and copies only when it is not.

2/2 also covers the headroom that afiucv_swap_src_dest() pushes without
checking. That started as a third patch; it collapsed into this one once it was
clear skb_cow_head() is needed for writability anyway and asking it for ETH_HLEN
rather than nothing is free.

2/2 was surfaced by the sashiko.dev review bot on the thread of a previous af_iucv
patch, which flagged both the missing unshare and the unchecked push; the Closes:
trailer on that patch points at its report.

By inspection; not reproduced. Compile-tested for s390x.

---
Bryam Vargas (2):
      net/iucv: drop HiperSockets frames from other network namespaces
      net/iucv: take a private, writable frame before rewriting it in place

 net/iucv/af_iucv.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
---
base-commit: a59f57e2aa127c5354168d2ec4bac920df1be4f4
change-id: 20260815-b4-disp-dc82fde4-3cae5e52ae9f

Best regards,
--  
Bryam Vargas <hexlabsecurity@proton.me>

[PATCH net 1/2] net/iucv: drop HiperSockets frames from other network namespaces

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

iucv_packet_type sets neither .dev nor .af_packet_net, so it lands in the
machine-global ptype_base[] that __netif_receive_skb_core walks for every
frame in every namespace, and afiucv_hs_rcv() ignores its dev argument.
An ETH_P_AF_IUCV frame sent from any namespace holding CAP_NET_RAW is
therefore matched against the global iucv_sk_list and can move a socket
owned by the initial namespace: afiucv_hs_callback_synfin() and _fin()
overwrite its sk_state, and _syn() builds an accept-queue child.

Filter on the namespace. The core does not do it for ptype_base[] --
net/core/dev.c leaves namespace filtering to the ptype owner -- and
net/x25/x25_dev.c and net/ieee802154/socket.c both test dev_net(dev) at
exactly this point.

Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/iucv/af_iucv.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index ea047bab65e7..e3ec965d96ca 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -2064,6 +2064,11 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
 	int err = NET_RX_SUCCESS;
 	char nullstring[8];
 
+	if (!net_eq(dev_net(dev), &init_net)) {
+		kfree_skb(skb);
+		return NET_RX_SUCCESS;
+	}
+
 	if (!pskb_may_pull(skb, sizeof(*trans_hdr))) {
 		kfree_skb(skb);
 		return NET_RX_SUCCESS;

-- 
2.55.0

[PATCH net 1/2] net/iucv: drop HiperSockets frames from other network namespaces

[Bryam Vargas]

iucv_packet_type sets neither .dev nor .af_packet_net, so it lands in the
machine-global ptype_base[] that __netif_receive_skb_core walks for every
frame in every namespace, and afiucv_hs_rcv() ignores its dev argument.
An ETH_P_AF_IUCV frame sent from any namespace holding CAP_NET_RAW is
therefore matched against the global iucv_sk_list and can move a socket
owned by the initial namespace: afiucv_hs_callback_synfin() and _fin()
overwrite its sk_state, and _syn() builds an accept-queue child.

Filter on the namespace. The core does not do it for ptype_base[] --
net/core/dev.c leaves namespace filtering to the ptype owner -- and
net/x25/x25_dev.c and net/ieee802154/socket.c both test dev_net(dev) at
exactly this point.

Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/iucv/af_iucv.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index ea047bab65e7..e3ec965d96ca 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -2064,6 +2064,11 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
 	int err = NET_RX_SUCCESS;
 	char nullstring[8];
 
+	if (!net_eq(dev_net(dev), &init_net)) {
+		kfree_skb(skb);
+		return NET_RX_SUCCESS;
+	}
+
 	if (!pskb_may_pull(skb, sizeof(*trans_hdr))) {
 		kfree_skb(skb);
 		return NET_RX_SUCCESS;

-- 
2.55.0

Re: [PATCH net 1/2] net/iucv: drop HiperSockets frames from other network namespaces

[sashiko-bot]

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

Pre-existing issues:
- [Critical] AF_IUCV allows unprivileged users in non-initial network namespaces to create sockets in the initial network namespace, bypassing namespace isolation.
- [High] Netdevice reference count leak during NETDEV_UNREGISTER leads to kernel hangs.
- [High] Bypass of `lock_sock()` mutual exclusion in softirq receive path leads to data corruption.
- [High] TOCTOU race in `afiucv_hs_callback_rx` leads to silent SKB memory leak.
- [High] Newly allocated IUCV sockets leak memory and references when an error occurs during connection request processing.
- [High] Incoming packets bypass `sk_rcvbuf` limits via unbounded queuing to `backlog_skb_q`, leading to OOM.
--

--- Patch [1]: [PATCH net 1/2] net/iucv: drop HiperSockets frames from other network namespaces ---
commit f9050fb5eddf9fc6d107045a06d53a731b334db3
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/iucv: drop HiperSockets frames from other network namespaces

This commit adds a namespace check to afiucv_hs_rcv() to drop HiperSockets
frames from other network namespaces. Because iucv_packet_type lacks specific
namespace bindings, it previously landed in the global ptype_base[], allowing
frames from namespaces holding CAP_NET_RAW to interact with sockets owned by
the initial namespace.

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index ea047bab65e7c..e3ec965d96ca7 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -2064,6 +2064,11 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
>  	int err = NET_RX_SUCCESS;
>  	char nullstring[8];
>  
> +	if (!net_eq(dev_net(dev), &init_net)) {
> +		kfree_skb(skb);
> +		return NET_RX_SUCCESS;
> +	}
> +
>  	if (!pskb_may_pull(skb, sizeof(*trans_hdr))) {
>  		kfree_skb(skb);
>  		return NET_RX_SUCCESS;

[Severity: Critical]
This is a pre-existing issue, but does iucv_sock_create() bypass namespace
isolation?

A process in a non-initial network namespace calling socket(AF_IUCV, ...)
ignores the passed network namespace and unconditionally allocates the socket
in init_net:

net/iucv/af_iucv.c:iucv_sock_create() {
    ...
    sk = iucv_sock_alloc(sock, protocol, GFP_KERNEL, kern);
    // inside iucv_sock_alloc:
    sk = sk_alloc(&init_net, PF_IUCV, prio, &iucv_proto, kern);
    ...
}

Subsequently, iucv_sock_bind() iterates through init_net devices and binds the
socket to them. This appears to allow local users in containers with socket
creation privileges to bypass namespace isolation and communicate over init_net
devices.

[Severity: High]
This isn't a bug introduced by this patch, but is there a netdevice reference
count leak during NETDEV_UNREGISTER?

When an AF_IUCV socket binds to a HiperSockets device via iucv_sock_bind(),
it takes a reference to the device using dev_hold(dev). The netdevice
notifier explicitly ignores the NETDEV_UNREGISTER event:

net/iucv/af_iucv.c:afiucv_netdev_event() {
    ...
    case NETDEV_DOWN:
    case NETDEV_UNREGISTER:
    default:
        break;
    ...
}

If a device is unregistered while a socket remains open, it seems the
unreleased reference could prevent the device from being fully removed,
causing the kernel to hang indefinitely in netdev_wait_allrefs().

[Severity: High]
This is a pre-existing issue, but does afiucv_hs_callback_syn() bypass
lock_sock() mutual exclusion?

The HiperSockets receive path (afiucv_hs_rcv) executes in softirq context.
Callbacks acquire bh_lock_sock(sk) but fail to check sock_owned_by_user(sk):

net/iucv/af_iucv.c:afiucv_hs_callback_syn() {
    ...
    bh_lock_sock(sk);
    if ((sk->sk_state != IUCV_LISTEN) || ...
    ...
}

If user context is currently holding lock_sock() (e.g., executing close()),
sk->sk_lock.owned is 1, but the softirq ignores this and modifies the socket
state concurrently. Could this lead to data races and socket state corruption?

[Severity: High]
This isn't a bug introduced by this patch, but is there a TOCTOU race in
afiucv_hs_callback_rx() leading to a silent SKB memory leak?

The code locklessly checks sk->sk_state:

net/iucv/af_iucv.c:afiucv_hs_callback_rx() {
    ...
    if (sk->sk_state != IUCV_CONNECTED) {
        kfree_skb(skb);
        return NET_RX_SUCCESS;
    }
    ...
}

Concurrently, iucv_sock_close() can execute, set the state to IUCV_CLOSED,
and purge backlog_skb_q. If the softirq reads the state as IUCV_CONNECTED
before it is updated, but queues the SKB after iucv_sock_close() has purged
the queue, the SKB lands in backlog_skb_q of a closing socket.

Since iucv_sock_destruct() fails to purge backlog_skb_q, is the SKB
permanently leaked?

[Severity: High]
This is a pre-existing issue, but do newly allocated IUCV sockets leak memory
and references when an error occurs during connection request processing?

In afiucv_hs_callback_syn(), a new socket is allocated via iucv_sock_alloc(),
which clears its SOCK_ZAPPED flag. On the error path, it calls
iucv_sock_kill(nsk):

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

However, iucv_sock_kill() returns immediately if !sock_flag(sk, SOCK_ZAPPED):

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

Since the socket is neither unlinked nor freed, can a remote sender exploit
this by spamming AF_IUCV SYN packets to exhaust kernel memory?

[Severity: High]
This isn't a bug introduced by this patch, but do incoming packets bypass
sk_rcvbuf limits?

Incoming data packets are appended to the socket's receive queue via
__sock_queue_rcv_skb(). If this fails because the receive buffer limit is
exceeded, the packet is queued to iucv->backlog_skb_q:

net/iucv/af_iucv.c:afiucv_hs_callback_rx() {
    ...
    if (__sock_queue_rcv_skb(sk, skb))
        /* handle rcv queue full */
        skb_queue_tail(&iucv->backlog_skb_q, skb);
    ...
}

Since backlog_skb_q lacks length limits, could a remote attacker sending a
continuous stream of packets force the kernel to continuously allocate SKBs
and bypass flow control limits?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260815-b4-disp-dc82fde4-v1-0-e83b10b22ce9@proton.me?part=1

Re: [PATCH net 1/2] net/iucv: drop HiperSockets frames from other network namespaces

[Alexandra Winter]

On 15.08.26 18:07, Bryam Vargas via B4 Relay wrote:
> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> iucv_packet_type sets neither .dev nor .af_packet_net, so it lands in the
> machine-global ptype_base[] that __netif_receive_skb_core walks for every
> frame in every namespace, and afiucv_hs_rcv() ignores its dev argument.
> An ETH_P_AF_IUCV frame sent from any namespace holding CAP_NET_RAW is
> therefore matched against the global iucv_sk_list and can move a socket
> owned by the initial namespace: afiucv_hs_callback_synfin() and _fin()
> overwrite its sk_state, and _syn() builds an accept-queue child.
> 
> Filter on the namespace. The core does not do it for ptype_base[] --
> net/core/dev.c leaves namespace filtering to the ptype owner -- and
> net/x25/x25_dev.c and net/ieee802154/socket.c both test dev_net(dev) at
> exactly this point.
> 
> Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
> ---
>  net/iucv/af_iucv.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index ea047bab65e7..e3ec965d96ca 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -2064,6 +2064,11 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
>  	int err = NET_RX_SUCCESS;
>  	char nullstring[8];
>  
> +	if (!net_eq(dev_net(dev), &init_net)) {
> +		kfree_skb(skb);
> +		return NET_RX_SUCCESS;
> +	}
> +
>  	if (!pskb_may_pull(skb, sizeof(*trans_hdr))) {
>  		kfree_skb(skb);
>  		return NET_RX_SUCCESS;
> 

I am wondering whether a check of
+		if (iucv_sk(sk)->hs_dev != dev)
+			continue;

would cover a broader range of issues.
I'll try to send a proper patch proposal.

Re: [PATCH net 1/2] net/iucv: drop HiperSockets frames from other network namespaces

[Bryam Vargas]

Alexandra,

> I am wondering whether a check of
> +		if (iucv_sk(sk)->hs_dev != dev)
> +			continue;
>
> would cover a broader range of issues.

It does, and I'd rather have yours than mine. It covers three things at
once: the namespace case, since hs_dev can only come from the init_net scan
in iucv_sock_bind(); the transport case the earlier patch went after, since
classic sockets have hs_dev == NULL and drop out of the walk; and delivery
to a socket bound to a different HiperSockets device.

I went looking for the regression it could carry -- an accept-queue child
left without hs_dev, which would break connection setup -- and it isn't
there. The child inherits at af_iucv.c:1908.

One thing it doesn't reach, and it's why I'm not dropping both: the check
sits after EBCASC() has already rewritten the transport header in place at
:2073-2076, and a SYN matching no socket still takes the !iucv branch at
:1872-1877, which swaps the frame and hands it to dev_queue_xmit(). So it
replaces 1/2 and stays complementary to 2/2.

Send yours and I'll drop 1/2.

Thanks,
Bryam

Re: [PATCH net 1/2] net/iucv: drop HiperSockets frames from other network namespaces

[Alexandra Winter]

On 21.08.26 13:42, Bryam Vargas wrote:
> Alexandra,
> 
>> I am wondering whether a check of
>> +		if (iucv_sk(sk)->hs_dev != dev)
>> +			continue;
>>
>> would cover a broader range of issues.
> 
> It does, and I'd rather have yours than mine. It covers three things at
> once: the namespace case, since hs_dev can only come from the init_net scan
> in iucv_sock_bind(); the transport case the earlier patch went after, since
> classic sockets have hs_dev == NULL and drop out of the walk; and delivery
> to a socket bound to a different HiperSockets device.
> 
> I went looking for the regression it could carry -- an accept-queue child
> left without hs_dev, which would break connection setup -- and it isn't
> there. The child inherits at af_iucv.c:1908.
> 
> One thing it doesn't reach, and it's why I'm not dropping both: the check
> sits after EBCASC() has already rewritten the transport header in place at
> :2073-2076, and a SYN matching no socket still takes the !iucv branch at
> :1872-1877, which swaps the frame and hands it to dev_queue_xmit(). So it
> replaces 1/2 and stays complementary to 2/2.
> 
> Send yours and I'll drop 1/2.
> 
> Thanks,
> Bryam
> 


Thank you very much Bryam, I fully agree.

I've sent R-b for 2/2, and propose to take it as it is.

See https://lore.kernel.org/netdev/20260821125501.3718748-1-wintera@linux.ibm.com/
for my proposal that should supersede 1/2.
Your review would be highly appreciated.

[PATCH net 2/2] net/iucv: take a private, writable frame before rewriting it in place

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

afiucv_hs_rcv() rewrites the frame in place -- EBCASC() converts four name
fields in the transport header, afiucv_swap_src_dest() swaps them and
pushes an Ethernet header back on -- without taking a private, writable
copy. It sits on the global ptype_base[], so a packet socket (tcpdump is
enough) has packet_rcv() clone every frame first, and net/core/dev.c has
warned since 1998 that such a handler "is not able to sense, that packet
is cloned and should be copied-on-write".

Unshare, then cow the head, in that order: skb_cow_head() can reach
pskb_expand_head(), which has BUG_ON(skb_shared()). Asking for ETH_HLEN
also covers the unchecked push in afiucv_swap_src_dest() --
eth_type_trans() has already pulled that much on the ordinary path, so the
call compares and returns.

Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
Closes: https://sashiko.dev/#/patchset/20260813-b4-disp-60433a46-v1-1-509e1200533e@proton.me?part=1
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/iucv/af_iucv.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index e3ec965d96ca..10cfc5e82f04 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -2069,11 +2069,20 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
 		return NET_RX_SUCCESS;
 	}
 
+	skb = skb_share_check(skb, GFP_ATOMIC);
+	if (!skb)
+		return NET_RX_SUCCESS;
+
 	if (!pskb_may_pull(skb, sizeof(*trans_hdr))) {
 		kfree_skb(skb);
 		return NET_RX_SUCCESS;
 	}
 
+	if (skb_cow_head(skb, ETH_HLEN)) {
+		kfree_skb(skb);
+		return NET_RX_SUCCESS;
+	}
+
 	trans_hdr = iucv_trans_hdr(skb);
 	EBCASC(trans_hdr->destAppName, sizeof(trans_hdr->destAppName));
 	EBCASC(trans_hdr->destUserID, sizeof(trans_hdr->destUserID));

-- 
2.55.0

[PATCH net 2/2] net/iucv: take a private, writable frame before rewriting it in place

[Bryam Vargas]

afiucv_hs_rcv() rewrites the frame in place -- EBCASC() converts four name
fields in the transport header, afiucv_swap_src_dest() swaps them and
pushes an Ethernet header back on -- without taking a private, writable
copy. It sits on the global ptype_base[], so a packet socket (tcpdump is
enough) has packet_rcv() clone every frame first, and net/core/dev.c has
warned since 1998 that such a handler "is not able to sense, that packet
is cloned and should be copied-on-write".

Unshare, then cow the head, in that order: skb_cow_head() can reach
pskb_expand_head(), which has BUG_ON(skb_shared()). Asking for ETH_HLEN
also covers the unchecked push in afiucv_swap_src_dest() --
eth_type_trans() has already pulled that much on the ordinary path, so the
call compares and returns.

Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
Closes: https://sashiko.dev/#/patchset/20260813-b4-disp-60433a46-v1-1-509e1200533e@proton.me?part=1
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/iucv/af_iucv.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index e3ec965d96ca..10cfc5e82f04 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -2069,11 +2069,20 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
 		return NET_RX_SUCCESS;
 	}
 
+	skb = skb_share_check(skb, GFP_ATOMIC);
+	if (!skb)
+		return NET_RX_SUCCESS;
+
 	if (!pskb_may_pull(skb, sizeof(*trans_hdr))) {
 		kfree_skb(skb);
 		return NET_RX_SUCCESS;
 	}
 
+	if (skb_cow_head(skb, ETH_HLEN)) {
+		kfree_skb(skb);
+		return NET_RX_SUCCESS;
+	}
+
 	trans_hdr = iucv_trans_hdr(skb);
 	EBCASC(trans_hdr->destAppName, sizeof(trans_hdr->destAppName));
 	EBCASC(trans_hdr->destUserID, sizeof(trans_hdr->destUserID));

-- 
2.55.0

Re: [PATCH net 2/2] net/iucv: take a private, writable frame before rewriting it in place

[sashiko-bot]

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

Pre-existing issues:
- [High] Unprotected socket state modification from softirq context
--

--- Patch [2]: [PATCH net 2/2] net/iucv: take a private, writable frame before rewriting it in place ---
commit 656ba922e59efdf102e7aa3950857e3c0061f9b8
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/iucv: take a private, writable frame before rewriting it in place

This commit modifies afiucv_hs_rcv() to take a private, writable copy
of the socket buffer before rewriting the frame in place. It unshares
the skb and uses skb_cow_head() to ensure enough headroom.

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

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index e3ec965d96ca7..10cfc5e82f047 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -2069,11 +2069,20 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
>  		return NET_RX_SUCCESS;
>  	}
>  

[ ... ]

[Severity: High]
This is a pre-existing issue, but while reviewing this file I noticed
an unprotected socket state modification:

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);
        }
        ...
}

Does this modification risk corrupting the socket state?

When qeth hardware issues TX completions, qeth_notify_skbs() is called from
softirq (NAPI) context and invokes afiucv_hs_callback_txnotify().

This modifies sk->sk_state directly without acquiring the socket lock
or checking sock_owned_by_user().

Could this lead to missed state transitions or concurrent execution with
process-context operations that assume exclusive access under lock_sock()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260815-b4-disp-dc82fde4-v1-0-e83b10b22ce9@proton.me?part=2

Re: [PATCH net 2/2] net/iucv: take a private, writable frame before rewriting it in place

[Alexandra Winter]

On 15.08.26 18:07, Bryam Vargas via B4 Relay wrote:
> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> afiucv_hs_rcv() rewrites the frame in place -- EBCASC() converts four name
> fields in the transport header, afiucv_swap_src_dest() swaps them and
> pushes an Ethernet header back on -- without taking a private, writable
> copy. It sits on the global ptype_base[], so a packet socket (tcpdump is
> enough) has packet_rcv() clone every frame first, and net/core/dev.c has
> warned since 1998 that such a handler "is not able to sense, that packet
> is cloned and should be copied-on-write".
> 
> Unshare, then cow the head, in that order: skb_cow_head() can reach
> pskb_expand_head(), which has BUG_ON(skb_shared()). Asking for ETH_HLEN
> also covers the unchecked push in afiucv_swap_src_dest() --
> eth_type_trans() has already pulled that much on the ordinary path, so the
> call compares and returns.
> 
> Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
> Closes: https://sashiko.dev/#/patchset/20260813-b4-disp-60433a46-v1-1-509e1200533e@proton.me?part=1
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
> ---

Thank you for this patch Bryam. Actually Hidayath proposed the same fix to me for the same Sashiko finding
and I asked him to clarify in the description what the consequences of today's problem are.

"My current understanding: You use an AF_PACKET ring reader to analyze the skbs received by HS L3 interface and
complain, that the skb is changed afterwards when af_iucv processes it.
Is that correct?
What could be the bad consequences? (I cannot think of any)"

Excuse my ignorance, if it is obvious to other readers, but is the worst thing that the output of tcpdump
is not correct? Is this really a problem fix then? Or should it go to net-next?




>  net/iucv/af_iucv.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index e3ec965d96ca..10cfc5e82f04 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -2069,11 +2069,20 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
>  		return NET_RX_SUCCESS;
>  	}
>  
> +	skb = skb_share_check(skb, GFP_ATOMIC);
> +	if (!skb)
> +		return NET_RX_SUCCESS;
> +
>  	if (!pskb_may_pull(skb, sizeof(*trans_hdr))) {
>  		kfree_skb(skb);
>  		return NET_RX_SUCCESS;
>  	}
>  
> +	if (skb_cow_head(skb, ETH_HLEN)) {
> +		kfree_skb(skb);
> +		return NET_RX_SUCCESS;
> +	}
> +
>  	trans_hdr = iucv_trans_hdr(skb);
>  	EBCASC(trans_hdr->destAppName, sizeof(trans_hdr->destAppName));
>  	EBCASC(trans_hdr->destUserID, sizeof(trans_hdr->destUserID));
> 

Re: [PATCH net 2/2] net/iucv: take a private, writable frame before rewriting it in place

[Bryam Vargas]

Alexandra,

> Excuse my ignorance, if it is obvious to other readers, but is the worst
> thing that the output of tcpdump is not correct?

Not obvious, and my description is why: it led with tcpdump, which is the
mildest end of this.

The order is the other way round. __netif_receive_skb_core() walks
ptype_base[] at net/core/dev.c:6160, before net->ptype_specific (:6169)
and orig_dev->ptype_specific (:6173). iucv_packet_type sets no .dev and no
.af_packet_net, so it sits in ptype_base[] while a packet socket for
ETH_P_AF_IUCV lands in one of the later lists. af_iucv runs first, and the
AF_PACKET reader gets the frame after EBCASC() has rewritten the four name
fields. The capture is wrong, but it was already wrong before the reader
was reached.

That isn't what I'd defend the patch on. Because af_iucv isn't the last
matching handler in that configuration, deliver_ptype_list_skb() hands it
over through deliver_skb(), which does refcount_inc(&skb->users) before
calling us (dev.c:2492, :2507). We run with users == 2, and on that skb we
rewrite the header in place, skb_push() 14 bytes in afiucv_swap_src_dest()
and pass the same skb to dev_queue_xmit() (af_iucv.c:1876, :1888, :1914) --
including for a frame that matched no socket (:1872).

What hides it in review is a guard asymmetry. deliver_skb() leaves
users == 2 with skb->cloned == 0, so skb_shared() is true while
skb_cloned() is false, and the copy-on-write guards all test skb_cloned()
-- __pskb_pull_tail() at skbuff.c:2886 among them -- so they read the skb
as already writable. The one that does test it is BUG_ON(skb_shared(skb))
at the top of pskb_expand_head() (skbuff.c:2305); skb_expand_head() carries
"/* pskb_expand_head() might crash, if skb is shared. */" (:2456) for the
same reason.

What I don't have is a panic. On the qeth geometry the first
pskb_may_pull() finds enough tailroom in the napi_get_frags() head and
copies out of the frags without expanding, so it doesn't reach
pskb_expand_head that way. By inspection; not reproduced.

> Is this really a problem fix then? Or should it go to net-next?

If the bar is a failure I can show you, net-next is right. I sent it to net
because a handler that writes a shared skb and then gives it to the
transmit path is a rule violation with a BUG_ON behind it, not because I
can fire that BUG_ON. Your call either way, and net-next is fine by me.

Worth having in the record: reaching the shared state costs one syscall --
socket(AF_PACKET, SOCK_RAW, htons(0xFBFB)), no bind, no ETH_P_ALL -- since
ptype_base[] is walked before the per-namespace list.

If Hidayath's version is further along, take his. I'd rather the check land
than land mine.

Thanks,
Bryam

Re: [PATCH net 2/2] net/iucv: take a private, writable frame before rewriting it in place

[Hidayath Khan]

On 21/08/26 5:12 pm, Bryam Vargas wrote:
> Alexandra,
>
>> Excuse my ignorance, if it is obvious to other readers, but is the worst
>> thing that the output of tcpdump is not correct?
> Not obvious, and my description is why: it led with tcpdump, which is the
> mildest end of this.
>
> The order is the other way round. __netif_receive_skb_core() walks
> ptype_base[] at net/core/dev.c:6160, before net->ptype_specific (:6169)
> and orig_dev->ptype_specific (:6173). iucv_packet_type sets no .dev and no
> .af_packet_net, so it sits in ptype_base[] while a packet socket for
> ETH_P_AF_IUCV lands in one of the later lists. af_iucv runs first, and the
> AF_PACKET reader gets the frame after EBCASC() has rewritten the four name
> fields. The capture is wrong, but it was already wrong before the reader
> was reached.
>
> That isn't what I'd defend the patch on. Because af_iucv isn't the last
> matching handler in that configuration, deliver_ptype_list_skb() hands it
> over through deliver_skb(), which does refcount_inc(&skb->users) before
> calling us (dev.c:2492, :2507). We run with users == 2, and on that skb we
> rewrite the header in place, skb_push() 14 bytes in afiucv_swap_src_dest()
> and pass the same skb to dev_queue_xmit() (af_iucv.c:1876, :1888, :1914) --
> including for a frame that matched no socket (:1872).
>
> What hides it in review is a guard asymmetry. deliver_skb() leaves
> users == 2 with skb->cloned == 0, so skb_shared() is true while
> skb_cloned() is false, and the copy-on-write guards all test skb_cloned()
> -- __pskb_pull_tail() at skbuff.c:2886 among them -- so they read the skb
> as already writable. The one that does test it is BUG_ON(skb_shared(skb))
> at the top of pskb_expand_head() (skbuff.c:2305); skb_expand_head() carries
> "/* pskb_expand_head() might crash, if skb is shared. */" (:2456) for the
> same reason.
>
> What I don't have is a panic. On the qeth geometry the first
> pskb_may_pull() finds enough tailroom in the napi_get_frags() head and
> copies out of the frags without expanding, so it doesn't reach
> pskb_expand_head that way. By inspection; not reproduced.
>
>> Is this really a problem fix then? Or should it go to net-next?
> If the bar is a failure I can show you, net-next is right. I sent it to net
> because a handler that writes a shared skb and then gives it to the
> transmit path is a rule violation with a BUG_ON behind it, not because I
> can fire that BUG_ON. Your call either way, and net-next is fine by me.
>
> Worth having in the record: reaching the shared state costs one syscall --
> socket(AF_PACKET, SOCK_RAW, htons(0xFBFB)), no bind, no ETH_P_ALL -- since
> ptype_base[] is walked before the per-namespace list.
>
> If Hidayath's version is further along, take his. I'd rather the check land
> than land mine.
Hi Bryam,

Please go ahead with your patch. I had dropped my patch and am not 
pursuing it.

Thanks,
Hidayath
>
> Thanks,
> Bryam
>

Re: [PATCH net 2/2] net/iucv: take a private, writable frame before rewriting it in place

[Alexandra Winter]

On 21.08.26 16:55, Hidayath Khan wrote:
> 
> On 21/08/26 5:12 pm, Bryam Vargas wrote:
>> Alexandra,
>>
>>> Excuse my ignorance, if it is obvious to other readers, but is the worst
>>> thing that the output of tcpdump is not correct?
>> Not obvious, and my description is why: it led with tcpdump, which is the
>> mildest end of this.
>>
>> The order is the other way round. __netif_receive_skb_core() walks
>> ptype_base[] at net/core/dev.c:6160, before net->ptype_specific (:6169)
>> and orig_dev->ptype_specific (:6173). iucv_packet_type sets no .dev and no
>> .af_packet_net, so it sits in ptype_base[] while a packet socket for
>> ETH_P_AF_IUCV lands in one of the later lists. af_iucv runs first, and the
>> AF_PACKET reader gets the frame after EBCASC() has rewritten the four name
>> fields. The capture is wrong, but it was already wrong before the reader
>> was reached.
>>
>> That isn't what I'd defend the patch on. Because af_iucv isn't the last
>> matching handler in that configuration, deliver_ptype_list_skb() hands it
>> over through deliver_skb(), which does refcount_inc(&skb->users) before
>> calling us (dev.c:2492, :2507). We run with users == 2, and on that skb we
>> rewrite the header in place, skb_push() 14 bytes in afiucv_swap_src_dest()
>> and pass the same skb to dev_queue_xmit() (af_iucv.c:1876, :1888, :1914) --
>> including for a frame that matched no socket (:1872).
>>
>> What hides it in review is a guard asymmetry. deliver_skb() leaves
>> users == 2 with skb->cloned == 0, so skb_shared() is true while
>> skb_cloned() is false, and the copy-on-write guards all test skb_cloned()
>> -- __pskb_pull_tail() at skbuff.c:2886 among them -- so they read the skb
>> as already writable. The one that does test it is BUG_ON(skb_shared(skb))
>> at the top of pskb_expand_head() (skbuff.c:2305); skb_expand_head() carries
>> "/* pskb_expand_head() might crash, if skb is shared. */" (:2456) for the
>> same reason.
>>
>> What I don't have is a panic. On the qeth geometry the first
>> pskb_may_pull() finds enough tailroom in the napi_get_frags() head and
>> copies out of the frags without expanding, so it doesn't reach
>> pskb_expand_head that way. By inspection; not reproduced.
>>
>>> Is this really a problem fix then? Or should it go to net-next?
>> If the bar is a failure I can show you, net-next is right. I sent it to net
>> because a handler that writes a shared skb and then gives it to the
>> transmit path is a rule violation with a BUG_ON behind it, not because I
>> can fire that BUG_ON. Your call either way, and net-next is fine by me.
>>
>> Worth having in the record: reaching the shared state costs one syscall --
>> socket(AF_PACKET, SOCK_RAW, htons(0xFBFB)), no bind, no ETH_P_ALL -- since
>> ptype_base[] is walked before the per-namespace list.
>>
>> If Hidayath's version is further along, take his. I'd rather the check land
>> than land mine.
> Hi Bryam,
> 
> Please go ahead with your patch. I had dropped my patch and am not pursuing it.
> 
> Thanks,
> Hidayath
>>
>> Thanks,
>> Bryam

Thank you for your explanations, Bryam.
I agree it makes sense to treat this as a fix.

Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>