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

On 15.08.26 18:07, Bryam Vargas via B4 Relay wrote:
> From: Bryam Vargas <[email protected]>
> 
> 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/[email protected]?part=1
> Signed-off-by: Bryam Vargas <[email protected]>
> ---

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