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

[Jakub Kicinski <kuba@kernel.org>]

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