Re: [PATCH net v3] net/iucv: fix the recvmsg window update

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Lockless modification of sk->sk_shutdown and sk->sk_state leads to data races and lost state updates.
- [High] Silent data truncation for SOCK_STREAM over HiperSockets when message size exceeds MTU.
--

commit 9ec78889adee8ffda3665e319fc36af7e52f542f
Author: Bryam Vargas <hexlabsecurity@proton.me>

    net/iucv: fix the recvmsg window update

    This commit attempts to fix a spurious disconnect in HiperSockets by
    properly checking the transport before sending an AF_IUCV_FLAG_WIN
    message. It also addresses a counter race by claiming the window credit
    after ruling out error exits.

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index 4e5cc9da6e068..db261ecd19af1 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -250,13 +244,22 @@ static int afiucv_hs_send(struct iucv_message *imsg, struct sock *sock,
>  	}
>  	skb->protocol = cpu_to_be16(ETH_P_AF_IUCV);

[Severity: High]
This is a pre-existing issue, but just above this point in afiucv_hs_send(),
large buffers are silently truncated:

	if (skb->len > skb->dev->mtu) {
		if (sock->sk_type == SOCK_SEQPACKET) {
			err = -EMSGSIZE;
			goto err_free;
		}
		err = pskb_trim(skb, skb->dev->mtu);
		if (err)
			goto err_free;
	}

If pskb_trim() succeeds for a SOCK_STREAM socket, afiucv_hs_send() returns
success. Does iucv_sock_sendmsg() then return the full original requested
length to user-space instead of the truncated length, masking the truncation
and causing silent data loss?

> +	/* Claim the receive credit here, not while building the header: every
> +	 * way this frame can be dropped has now been ruled out, so the window
> +	 * is zeroed only for as long as the transmit itself takes.
> +	 */
> +	if (flags == AF_IUCV_FLAG_WIN || !flags) {
> +		confirm_recv = atomic_xchg(&iucv->msg_recv, 0);
> +		phs_hdr->window = confirm_recv;
> +		if (confirm_recv)
> +			phs_hdr->flags = phs_hdr->flags | AF_IUCV_FLAG_WIN;
> +	}
> +
>  	atomic_inc(&iucv->skbs_in_xmit);
>  	err = dev_queue_xmit(skb);

[ ... ]

> @@ -1331,16 +1335,20 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
>  		if (skb_queue_empty(&iucv->backlog_skb_q)) {
>  			if (!list_empty(&iucv->message_q.list))
>  				iucv_process_message_q(sk);
> -			if (atomic_read(&iucv->msg_recv) >=
> -							iucv->msglimit / 2) {
> -				err = iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN);
> -				if (err) {
> -					sk->sk_state = IUCV_DISCONN;
> -					sk->sk_state_change(sk);
> -				}
> -			}
> +			if (iucv->transport == AF_IUCV_TRANS_HIPER &&
> +			    atomic_read(&iucv->msg_recv) >=
> +							iucv->msglimit / 2)
> +				send_win = true;
>  		}
>  		spin_unlock_bh(&iucv->message_q.lock);
> +
> +		if (send_win) {
> +			err = iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN);

[Severity: High]
This is a pre-existing issue, but does calling iucv_send_ctrl() here without
holding lock_sock() cause data races?

In net/iucv/af_iucv.c:iucv_send_ctrl():

	if (sk->sk_shutdown & SEND_SHUTDOWN) {
		/* controlling flags should be sent anyway */
		shutdown = sk->sk_shutdown;
		sk->sk_shutdown &= RCV_SHUTDOWN;
	}
[ ... ]
	if (shutdown)
		sk->sk_shutdown = shutdown;

Could this lockless temporary modification overwrite concurrent updates to
sk_shutdown (e.g., losing a RCV_SHUTDOWN flag set by shutdown()), leading to
an application hanging indefinitely in recvmsg?

> +			if (err) {
> +				sk->sk_state = IUCV_DISCONN;

[Severity: High]
This is also a pre-existing issue, but is it safe to update sk_state locklessly
here? This appears to violate core socket locking rules, potentially corrupting
the state machine.

> +				sk->sk_state_change(sk);
> +			}
> +		}
>  	}
>  
>  done:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-b4-disp-33fac0ed-v3-1-e6d061880ee0@proton.me?part=1