Re: [PATCH net v2 2/2] net/iucv: do not count the SHT frame against the receive window

[Alexandra Winter]

On 29.10.26 05:46, Bryam Vargas via B4 Relay wrote:
> From: Bryam Vargas <hexlabsecurity@xxxxxxxxx>
> 
> iucv_sock_recvmsg() counts every HiperSockets frame it consumes in
> msg_recv, the SHT's empty one included, while the peer counts only what
> sendmsg() sends against the window it was granted; the SHT goes out
> through iucv_send_ctrl() uncounted, one per shutdown(SHUT_WR) call. The
> next window update zeroes msg_recv and takes the surplus with it, but
> recvmsg() sends none while frames wait in backlog_skb_q, and an SHT read
> in that state pushes msg_recv past msglimit: WARN_ON(1) fires, the socket
> is closed and the reader gets -EFAULT instead of EOF. A peer that sends
> past its window reaches the same WARN from the network; with
> panic_on_warn that is a remote panic. The SHT frame dates from
> 82492a355fac, the WARN and the close from 800c5eb7b5eb.

I agree with the ananlysis, at least the WARN_ON needs to go away.
I will try to reproduce it.


> 
> The SHT reaches recvmsg() as a zero-length frame, like a zero-length
> data frame, so test the length: count only frames that still carry
> data when the read consumes them (rlen; a partial SOCK_STREAM read
> requeues above this point, so each data frame counts once), and close a
> socket over the window without the WARN; -EFAULT is kept and nothing is
> logged. 

I understand that after this patch this path can only be reached /
reproduced by a modified sender?

A zero-length data frame that reaches recvmsg() sets
> RCV_SHUTDOWN, after which afiucv_hs_callback_rx() drops every later
> data frame uncounted; the one slot this patch no longer credits for it
> is lost like the slots of the frames dropped after it.
> 
> The close over the window purges backlog_skb_q, not sk_receive_queue,
> and nothing resets msg_recv; once empty frames are not counted, a read
> of an empty frame from that queue after the close would skip the
> -EFAULT branch and reach the window update, where iucv_send_ctrl()
> sizes the skb through the cleared hs_dev. Send the update only while
> the socket is connected.

Shouldn't the 'HiperSockets receive path with the socket lock'
prevent this?

[...]
>  			}
> @@ -1357,7 +1360,11 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
>  		}
>  		spin_unlock_bh(&iucv->message_q.lock);
>  
> -		if (send_win) {
> +		/* iucv_sock_close() clears hs_dev but leaves sk_receive_queue
> +		 * readable: a frame read after the over-window close must not
> +		 * send a window update.
> +		 */
> +		if (send_win && sk->sk_state == IUCV_CONNECTED) {

without the socket lock, this can be false the next cycle, right?

>  			err = iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN);
>  			if (err) {
>  				sk->sk_state = IUCV_DISCONN;
>