[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 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 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

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