Re: [PATCH net] net/iucv: only deliver HiperSockets frames to HiperSockets sockets

On 13.08.26 14:51, Bryam Vargas via B4 Relay wrote:
> From: Bryam Vargas <[email protected]>
> 
> afiucv_hs_rcv() selects a socket out of iucv_sk_list by the four name
> fields alone, with no test on iucv->transport, so a frame arriving over
> HiperSockets can be delivered to a socket bound to the classic z/VM IUCV
> transport. iucv_sock_bind() makes that reachable rather than theoretical:
> a bind to the local guest userid always takes the classic path, even on a
> guest that also carries a HiperSockets device with the same identifier.
> 
> Skip sockets that are not on the HiperSockets transport. The two were
> added as alternatives for environments assumed disjoint - IUCV under
> z/VM, HiperSockets on LPAR - and this lookup still assumes a guest has
> only one of them.
> 
> Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
> Signed-off-by: Bryam Vargas <[email protected]>
> ---
> The two transports were introduced as alternatives for environments the
> 2011 series treated as disjoint. Its cover letter says so:
> 
>   "The current transport mechanism for af_iucv (iucv) is only available
>    on VM. HiperSockets provide similar capabilities as iucv and are
>    available on LPAR."
> 
>   https://lore.kernel.org/all/[email protected]/
> 
> That premise is the one to check, and it is yours to settle: on a z/VM guest
> that also has a HiperSockets device both exist at once, and iucv_sock_bind()
> resolves the local userid to the classic transport at the test against
> iucv_userid. If a guest can never reach both, this patch is unnecessary and
> I would rather know that than have it applied.
> 
> Reach is wider than the HiperSockets LAN, which is what decides how urgently
> this is worth taking: iucv_packet_type sets no .dev, afiucv_hs_rcv() ignores
> its dev argument, and nothing checks dev_net() -- net/x25/x25_dev.c and
> net/ieee802154/socket.c both do at exactly that point. An AF_PACKET frame on
> lo from any netns holding CAP_NET_RAW reaches these sockets.
> 
> Two consequences I traced on a classic socket that matches an inbound frame:
> afiucv_hs_callback_synfin() and _fin() overwrite its sk_state, and
> afiucv_hs_callback_syn() builds an accept-queue child with transport HIPER
> but hs_dev NULL, which LL_RESERVED_SPACE() dereferences unguarded on the
> first send. That read lands in mapped lowcore on a default kernel and
> afiucv_hs_send() then returns -ENODEV, so I am not claiming a panic; it
> faults with relocate_lowcore. By inspection; not reproduced. Compile-tested
> for s390x.
> 
> One case where this patch is a regression: a device whose hsuid is set to
> the same 8 characters as
> the guest's z/VM userid. iucv_sock_bind() tests siucv_user_id against
> iucv_userid before it scans for a HiperSockets device, so such a socket
> becomes classic and today receives HiperSockets frames only because this
> lookup does not filter. After this patch it stops receiving them. If that
> configuration is one you support, then this is the wrong patch and the fix
> belongs in the bind ordering.
> ---
>  net/iucv/af_iucv.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index ea047bab65e7..5fb6793b9a64 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)->transport != AF_IUCV_TRANS_HIPER)
> +			continue;
>  		if (trans_hdr->flags == AF_IUCV_FLAG_SYN) {
>  			if ((!memcmp(&iucv_sk(sk)->src_name,
>  				     trans_hdr->destAppName, 8)) &&
> 
> ---
> base-commit: 9006c116dd111d457bf5d074990210f70a4ad2c8
> change-id: 20260813-b4-disp-60433a46-fcdca197129a
> 
> Best regards,
> --  
> Bryam Vargas <[email protected]>

Hello Bryam,
I agree with your analysis and I welcome your patch, thank you.

As you rightly point out the two transports were introduced as alternatives,
and AF_IUCV over HiperSockets is not intended to work between z/VM guests.

Actually we should prevent setting hsuid in z/VM guests, altogether. That
would also be a good point in time to indicate to the user that they are
doing something meaningless. We'll work on such a fix.

You patch is still good as a protection against malformed packets arriving
at a HiperSockets L3 interface.

Reviewed-by: Alexandra Winter <[email protected]>