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

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