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

[Paolo Abeni <pabeni@redhat.com>]

On 10/8/26 11:21, Bryam Vargas via B4 Relay wrote:
> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> afiucv_hs_rcv() rewrites the frame in place -- EBCASC() on four name
> fields, then afiucv_swap_src_dest() swaps them and pushes an Ethernet
> header -- and can hand the same skb to dev_queue_xmit(), without making
> it private first, so a packet socket that also gets the frame reads the
> rewritten names. An ETH_P_ALL tap runs first and leaves af_iucv a
> cloned skb; a socket opened for ETH_P_AF_IUCV sits on a ptype_specific
> list, walked after ptype_base[], so deliver_skb() takes a reference and
> af_iucv runs on an skb that is shared but not cloned.
> 
> 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(); on the ordinary
> path eth_type_trans() has already pulled that much, so it only compares.
> 
> 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>
> Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>
> ---
> Sorry for the long silence since August: a major earthquake hit where I
> live, and I was finishing a postgrad program. Back now.
> 
> v2: the old 2/2 alone. 1/2 is dropped; Alexandra's 80230a18c164
>      ("net/iucv: filter frames in afiucv_hs_rcv() by ingress device")
>      replaced it and is in mainline. Rebased on net, no code change,
>      carries Alexandra's R-b. Kept on net; Alexandra agreed on v1 to
>      treat it as a fix. The commit message now covers both cases: v1
>      described the ETH_P_ALL tap, where af_iucv gets a cloned skb, and
>      the v1 thread added the ETH_P_AF_IUCV socket, where it gets a shared
>      one. My reply there said v1 had the delivery order backwards. It
>      didn't; both cases are real, and the A/B below shows each call is
>      needed.
> 
> v1: https://lore.kernel.org/all/20260815-b4-disp-dc82fde4-v1-0-e83b10b22ce9@proton.me/
> 
> Tested on s390x under QEMU TCG, KASAN and lockdep on. No z/VM there, so
> af_iucv gets its frames from lo (80230a18c164 filters after the
> EBCASC(), so lo frames still hit the rewrite), and a packet socket
> reads each one back. Unpatched, the tap and the ETH_P_AF_IUCV socket
> both get the 48 name bytes already in ASCII, and a SYN with no listener
> never reaches that socket as a 0xfbfb frame. Patched, every frame
> arrives as sent. Drop skb_share_check() and the ETH_P_AF_IUCV socket is
> broken again; drop skb_cow_head() and both are. FWIW no splat in any
> boot, patched or not -- on lo the damage is in what the other receivers
> read. Builds clean for s390 with W=1. Repro and logs on request.
> ---
>   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 db261ecd19af..493dc156e9f5 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -2072,11 +2072,20 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
>   	int err = NET_RX_SUCCESS;
>   	char nullstring[8];
>   
> +	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)) {

Sashiko (nipa) noted the above is the wrong helper:

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-b4-disp-bf46f1ba-v2-1-46ff1ea9c40b@proton.me

you should use skb_ensure_writable() or skb_cow()

/P