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

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

[Bryam Vargas]

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Unchecked `skb_push()` in `afiucv_swap_src_dest()` causes kernel panic (`skb_under_panic`) due to insufficient headroom.
- [High] In-place modification of potentially shared or cloned SKB payload in `afiucv_hs_rcv()`.
- [High] Invalid transmission of a shared/cloned RX SKB directly via `dev_queue_xmit()`.
- [High] Missing network namespace validation allows cross-namespace packet injection and processing.
- [High] Unprotected list iteration in `afiucv_netdev_event` without acquiring `iucv_sk_list.lock`.
- [High] `afiucv_hs_callback_txnotify` modifies socket state without holding the socket lock.
--

--- Patch [1]: [PATCH net] net/iucv: only deliver HiperSockets frames to HiperSockets sockets ---
commit c7ea9342df754a7a4600b67180607cae05a3e24c
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/iucv: only deliver HiperSockets frames to HiperSockets sockets
    
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.

> 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;
>  		if (trans_hdr->flags == AF_IUCV_FLAG_SYN) {
>  			if ((!memcmp(&iucv_sk(sk)->src_name,
>  				     trans_hdr->destAppName, 8)) &&

[Severity: High]
This is a pre-existing issue, but I noticed afiucv_hs_rcv() modifies the SKB
in-place without checking if it is shared or cloned:

net/iucv/af_iucv.c:afiucv_hs_rcv() {
    ...
    trans_hdr = iucv_trans_hdr(skb);
    EBCASC(trans_hdr->destAppName, sizeof(trans_hdr->destAppName));
    ...
}

Does this corrupt the payload for other consumers if the networking core
delivered the SKB as shared or cloned?

[Severity: High]
This is a pre-existing issue, but I noticed afiucv_hs_rcv() does not check the
network namespace before processing the packet:

net/iucv/af_iucv.c:afiucv_hs_rcv() {
    static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
                             struct packet_type *pt, struct net_device *orig_dev)
    {
        ...
    }
}

Could an unprivileged user with CAP_NET_RAW in a container namespace craft and
send ETH_P_AF_IUCV packets on their local dummy or loopback device, bypassing
namespace isolation?

[Severity: Critical]
This is a pre-existing issue, but I noticed an unchecked skb_push() that could
cause a panic:

net/iucv/af_iucv.c:afiucv_swap_src_dest() {
    ...
    skb_push(skb, ETH_HLEN);
    memset(skb->data, 0, ETH_HLEN);
}

When afiucv_hs_callback_syn() bounces unmatched SYN packets, it passes the
newly received SKB to afiucv_swap_src_dest(). If an attacker injects a SYN
packet with less than 14 bytes of headroom, will this unconditionally push into
the SKB headroom and trigger skb_under_panic()?

[Severity: High]
This is a pre-existing issue, but does afiucv_hs_callback_syn() pass a received
RX SKB directly to the TX path?

net/iucv/af_iucv.c:afiucv_hs_callback_syn() {
    ...
    afiucv_swap_src_dest(skb);
    trans_hdr->flags = AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_FIN;
    err = dev_queue_xmit(skb);
    ...
}

If the SKB was delivered as shared or cloned, will modifying and transmitting
it trigger a WARN_ON(1) in __dev_queue_xmit() and violate buffer handoff
safety?

[Severity: High]
This is a pre-existing issue, but does afiucv_netdev_event() iterate over the
socket list without acquiring the read lock?

net/iucv/af_iucv.c:afiucv_netdev_event() {
    ...
    case NETDEV_GOING_DOWN:
        sk_for_each(sk, &iucv_sk_list.head) {
            iucv = iucv_sk(sk);
            ...
}

While this callback runs with the RTNL lock held, can concurrent socket creation
or destruction (which only hold the iucv_sk_list.lock) modify the list and
cause a use-after-free or infinite loop here?

[Severity: High]
This is a pre-existing issue, but does afiucv_hs_callback_txnotify() modify the
socket state locklessly?

net/iucv/af_iucv.c:afiucv_hs_callback_txnotify() {
    ...
    if (sk->sk_state == IUCV_CONNECTED) {
        sk->sk_state = IUCV_DISCONN;
        sk->sk_state_change(sk);
    }
    ...
}

Since this is called by the underlying driver outside of process context, does
modifying the state without holding bh_lock_sock(sk) create data races with
other socket operations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813-b4-disp-60433a46-v1-1-509e1200533e@proton.me?part=1

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>