Re: [PATCH RFC net v3 1/7] net/iucv: keep the window given to the peer in a field of its own

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] net/iucv: premature and unconditional msglimit_adv write in
  afiucv_hs_send()
--

commit 8534ba296d230681179bb7448129eaebb9ecfc26
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/iucv: keep the window given to the peer in a field of its own

This commit records the receive window advertised in HiperSockets SYN and
SYN|ACK frames in a separate msglimit_adv field, and uses it in
iucv_sock_recvmsg() so later SO_MSGLIMIT changes do not alter window
accounting for that connection.

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index ce13fe3c85b52..657c61161dbdc 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c

[ ... ]

> @@ -208,8 +222,11 @@ static int afiucv_hs_send(struct iucv_message *imsg, struct sock *sock,
>  	phs_hdr->magic = ETH_P_AF_IUCV;
>  	phs_hdr->version = 1;
>  	phs_hdr->flags = flags;
> -	if (flags == AF_IUCV_FLAG_SYN)
> +	if (flags == AF_IUCV_FLAG_SYN) {
>  		phs_hdr->window = iucv->msglimit;
> +		/* what the peer may send us, whatever msglimit becomes */
> +		iucv->msglimit_adv = iucv->msglimit;
> +	}

[Severity: Medium]
Can setting iucv->msglimit_adv here cause it to diverge from the window the
peer actually received?

afiucv_hs_send() writes iucv->msglimit_adv before the device and MTU checks
and dev_queue_xmit(), and overwrites it on every AF_IUCV_FLAG_SYN call.
After a non-blocking or interrupted connect() sends a SYN and leaves
sk->sk_state as IUCV_BOUND, changing SO_MSGLIMIT and calling connect() again
overwrites iucv->msglimit_adv even if the peer's SYN|ACK to the first SYN is
already in flight or the second send fails with -ENETDOWN.  When the peer
sends frames up to the window from the first SYN, iucv_sock_recvmsg() then
hits WARN_ON(1), closes the socket and returns -EFAULT if SO_MSGLIMIT was
lowered, or fails to send AF_IUCV_FLAG_WIN updates if it was raised.

Similarly, if an initial connect() fails before transmitting a SYN and
SO_MSGLIMIT is changed before an incoming SYN|ACK connects the bound socket,
afiucv_hs_window() returns the stale msglimit_adv from the failed connect()
instead of iucv->msglimit.  In addition, iucv_sock_connect() checks
sk->sk_state before lock_sock(sk) without rechecking under the lock, so a
concurrent connect() can still enter afiucv_hs_send() and overwrite
iucv->msglimit_adv after the socket reaches IUCV_CONNECTED.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-b4-disp-2a6b4364-v3-0-35677b68e697@proton.me?part=1