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

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

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

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 <hexlabsecurity@proton.me>
---
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/20110727161339.530894848@de.ibm.com/

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 <hexlabsecurity@proton.me>

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

[Alexandra Winter]

On 13.08.26 14:51, Bryam Vargas via B4 Relay wrote:
> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> 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 <hexlabsecurity@proton.me>
> ---
> 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/20110727161339.530894848@de.ibm.com/
> 
> 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 <hexlabsecurity@proton.me>

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 <wintera@linux.ibm.com>

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email says "Looks like other places need a fix, too. Please fix them
in one patch, failing that in one series."
---
net/iucv: only deliver HiperSockets frames to HiperSockets sockets

This adds a transport test to the socket lookup in afiucv_hs_rcv() so that
an inbound HiperSockets frame skips sockets bound to the classic z/VM IUCV
transport.  Selection previously used only the four 8-byte name fields from
the frame header.

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index ea047bab65e7c..5fb6793b9a64a 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;

[Severity: Medium]

This isn't a bug introduced by this patch, but should the other lookup over
iucv_sk_list get the same treatment?  iucv_callback_connreq(), which handles
inbound classic z/VM IUCV connect requests, still matches on name alone:

net/iucv/af_iucv.c:iucv_callback_connreq() {
	sk_for_each(sk, &iucv_sk_list.head)
		if (sk->sk_state == IUCV_LISTEN &&
		    !memcmp(&iucv_sk(sk)->src_name, src_name, 8)) {
			iucv = iucv_sk(sk);
			break;
		}
}

On the configuration the commit message describes (pr_iucv set and a
HiperSockets device present, so both transports coexist), can a socket bound
through the HIPER branch of iucv_sock_bind() be picked here?

If so, the child it builds looks inconsistent:

net/iucv/af_iucv.c:iucv_callback_connreq() {
	niucv->transport = AF_IUCV_TRANS_IUCV;
	...
	memcpy(niucv->src_user_id, iucv->src_user_id, 8);
}

That is transport AF_IUCV_TRANS_IUCV with src_user_id copied from a HIPER
parent, i.e. the device hsuid, while the vm_bind path of iucv_sock_bind() and
iucv_sock_autobind() always set src_user_id to iucv_userid for classic
sockets.

After this change the two directions disagree about whether transport is part
of the demux key.  Was making only the HiperSockets side transport-aware
intentional?

[Severity: High]

This is a pre-existing issue, but since this hunk is the lookup in question:
should the ingress device and its namespace also be checked here?

afiucv_hs_rcv() never uses its dev or orig_dev arguments, and the handler is
registered globally:

net/iucv/af_iucv.c
	static struct packet_type iucv_packet_type = {
		.type = cpu_to_be16(ETH_P_AF_IUCV),
		.func = afiucv_hs_rcv,
	};
	...
	dev_add_pack(&iucv_packet_type);

With no .dev and no .af_packet_net, ptype_head() files this in the global
ptype_base[], and __netif_receive_skb_core() delivers ETH_P_AF_IUCV frames
from any net_device in any namespace:

net/core/dev.c:__netif_receive_skb_core() {
	deliver_ptype_list_skb(skb, &pt_prev, orig_dev, type,
			       &ptype_base[ntohs(type) &
					   PTYPE_HASH_MASK]);
}

Meanwhile every AF_IUCV socket lives in init_net:

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

Comparable ptype handlers filter at exactly this point (net/x25/x25_dev.c and
net/ieee802154/socket.c both test the namespace).

After this patch, is the only selection criteria still transport plus the four
attacker-supplied 8-byte header fields, with no comparison against
iucv->hs_dev or sk->sk_bound_dev_if, both of which iucv_sock_bind() sets for
HIPER sockets?

	sk->sk_bound_dev_if = dev->ifindex;
	iucv->hs_dev = dev;

Two things seem to follow.  First, can a matching frame arriving on device B
be delivered to a socket bound to device A, with afiucv_hs_callback_syn()
then creating a child that inherits the listener's device and replies out
through A?

net/iucv/af_iucv.c:afiucv_hs_callback_syn() {
	nsk->sk_bound_dev_if = sk->sk_bound_dev_if;
	niucv->hs_dev = iucv->hs_dev;
}

Second, can a process holding CAP_NET_RAW in an unprivileged user and net
namespace send a raw frame with ethertype 0xFBFB on its own lo and reach host
HIPER sockets?  loopback_xmit() re-derives skb->protocol via
eth_type_trans(), and a destination MAC of all zeros matches loopback's
dev_addr so pkt_type is PACKET_HOST.  Reaching the callbacks would mean
afiucv_hs_callback_synfin() and afiucv_hs_callback_fin() forcing
sk_state = IUCV_DISCONN with sk_state_change(), afiucv_hs_callback_rx()
queueing injected bytes on a connected socket:

net/iucv/af_iucv.c:afiucv_hs_callback_rx() {
	if (skb_queue_empty(&iucv->backlog_skb_q)) {
		if (__sock_queue_rcv_skb(sk, skb))
	...
}

plus afiucv_hs_callback_win() applying a caller-chosen window through
atomic_sub() on iucv->msg_sent, and afiucv_hs_callback_syn() adding children
to a listening socket's accept queue.

Given the subject line says "only deliver HiperSockets frames to HiperSockets
sockets", would it be worth either adding a dev_net(dev) and device test in
the same loop, or noting in the commit message that this path stays open?

The submission's own trailing notes rely on this vector when arguing that
reach extends beyond the HiperSockets LAN, so the recorded message and the
code appear to differ on what is being closed.
-- 
pw-bot: cr

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

[Alexandra Winter]

On 17.08.26 22:25, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email says "Looks like other places need a fix, too. Please fix them
> in one patch, failing that in one series."
> ---
> net/iucv: only deliver HiperSockets frames to HiperSockets sockets
> 
> This adds a transport test to the socket lookup in afiucv_hs_rcv() so that
> an inbound HiperSockets frame skips sockets bound to the classic z/VM IUCV
> transport.  Selection previously used only the four 8-byte name fields from
> the frame header.
> 
>> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
>> index ea047bab65e7c..5fb6793b9a64a 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;
> 
> [Severity: Medium]
> 
> This isn't a bug introduced by this patch, but should the other lookup over
> iucv_sk_list get the same treatment?  iucv_callback_connreq(), which handles
> inbound classic z/VM IUCV connect requests, still matches on name alone:
> 
> net/iucv/af_iucv.c:iucv_callback_connreq() {
> 	sk_for_each(sk, &iucv_sk_list.head)
> 		if (sk->sk_state == IUCV_LISTEN &&
> 		    !memcmp(&iucv_sk(sk)->src_name, src_name, 8)) {
> 			iucv = iucv_sk(sk);
> 			break;
> 		}
> }
> 
> On the configuration the commit message describes (pr_iucv set and a
> HiperSockets device present, so both transports coexist), can a socket bound
> through the HIPER branch of iucv_sock_bind() be picked here?
> 


The finding makes sense.
It is probably best to fix this for good by making the 2 environements mutually
exclusive, like it was intended.

Afaict the TRANS_IUCV can only be used in a z/VM environement.  "pr_iucv != NULL" guards that pretty well.
(To be verified)

So preventing TRANS_HIPER in a z/VM environement would be the fix.
Setting hsuid only in LPARs is one part. But a HS L3 interface could still exist in a z/VM environement, and
receive malformed data. So that needs to be taken care of as well.


My hope would be that such a clear cut between the environements would reduce the number
of scenarios and thus reduce the number of findings.

Thoughts anybody?


> If so, the child it builds looks inconsistent:
> 
> net/iucv/af_iucv.c:iucv_callback_connreq() {
> 	niucv->transport = AF_IUCV_TRANS_IUCV;
> 	...
> 	memcpy(niucv->src_user_id, iucv->src_user_id, 8);
> }
> 
> That is transport AF_IUCV_TRANS_IUCV with src_user_id copied from a HIPER
> parent, i.e. the device hsuid, while the vm_bind path of iucv_sock_bind() and
> iucv_sock_autobind() always set src_user_id to iucv_userid for classic
> sockets.
> 
> After this change the two directions disagree about whether transport is part
> of the demux key.  Was making only the HiperSockets side transport-aware
> intentional?
> 
> [Severity: High]
> 
> This is a pre-existing issue, but since this hunk is the lookup in question:
> should the ingress device and its namespace also be checked here?
> 
> afiucv_hs_rcv() never uses its dev or orig_dev arguments, and the handler is
> registered globally:
> 
> net/iucv/af_iucv.c
> 	static struct packet_type iucv_packet_type = {
> 		.type = cpu_to_be16(ETH_P_AF_IUCV),
> 		.func = afiucv_hs_rcv,
> 	};
> 	...
> 	dev_add_pack(&iucv_packet_type);
> 
> With no .dev and no .af_packet_net, ptype_head() files this in the global
> ptype_base[], and __netif_receive_skb_core() delivers ETH_P_AF_IUCV frames
> from any net_device in any namespace:
> 


Correct. A check is required that only IQD devices in an LPAR with hsuid are
valid receivers. Coordination with the qeth driver should be considered.
[...]

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

[Alexandra Winter]

On 13.08.26 14:51, Bryam Vargas via B4 Relay wrote:
> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> 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 <hexlabsecurity@proton.me>
> ---
> 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/20110727161339.530894848@de.ibm.com/
> 
> 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 <hexlabsecurity@proton.me>
> 
> 
> 

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.