Re: [PATCH net 1/2] net/iucv: only send the window update on HiperSockets sockets

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/iucv: only send the window update on HiperSockets sockets

This adds a transport test before iucv_sock_recvmsg() sends
AF_IUCV_FLAG_WIN, so that iucv_send_ctrl() is no longer called on classic
z/VM sockets where iucv->hs_dev is NULL.  Two questions below, one about
the impact statement in the changelog and one about the surrounding
msg_recv accounting.

>     iucv_sock_recvmsg() sends AF_IUCV_FLAG_WIN without testing the transport,
>     but that flag exists only on HiperSockets. On a classic z/VM socket
>     iucv->hs_dev is NULL and iucv_send_ctrl() sizes the skb from
>     LL_RESERVED_SPACE(iucv->hs_dev), so the read goes through NULL;
>     afiucv_hs_send() then returns -ENODEV and recvmsg() moves the socket to
>     IUCV_DISCONN. Reaching it takes an application that sets SO_MSGLIMIT to 1
>     on its own socket, since msg_recv is incremented only on the HiperSockets
>     path, so this is a correctness fix rather than a security one.

[Severity: Medium]
The bug being fixed here is pre-existing (3881ac441f64) and is not
introduced by this patch, but is the concluding sentence, "so this is a
correctness fix rather than a security one", accurate for a patch carrying
Cc: stable?

The trigger is entirely under the control of an unprivileged local process
using the ordinary socket API.  SO_MSGLIMIT accepts 1 in iucv_setsockopt():

	if (val < 1 || val > U16_MAX)
		rc = -EINVAL;
	else
		iucv->msglimit = val;

msg_recv stays at 0 on a classic socket, so msglimit / 2 == 0 makes the
old predicate 0 >= 0 on every successful non-PEEK recvmsg(), and
iucv_send_ctrl() then evaluates:

net/iucv/af_iucv.c:iucv_send_ctrl() {
	blen = sizeof(struct af_iucv_trans_hdr) +
	       LL_RESERVED_SPACE(iucv->hs_dev);

with hs_dev == NULL, before afiucv_hs_send() gets a chance to return
-ENODEV on its !skb->dev test.

The changelog itself then says "with lowcore relocation it faults", and
relocate_lowcore is a supported s390 boot parameter.  On such kernels an
ordinary recvmsg() from an unprivileged process oopses, which reads like a
local denial of service rather than a correctness issue.  Would it be worth
restating the impact so stable maintainers can prioritise and tag it
accordingly?

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index ea047bab65e7c..0bc4a15f4b565 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -1331,7 +1331,8 @@ 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) >=
> +			if (iucv->transport == AF_IUCV_TRANS_HIPER &&
> +			    atomic_read(&iucv->msg_recv) >=
>  							iucv->msglimit / 2) {
>  				err = iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN);
>  				if (err) {

[Severity: High]
This is a pre-existing issue and is not introduced by this patch, but since
the patch touches this exact accounting block, can the WARN a few lines
above be reached from remote input on the HiperSockets path?

Just before the hunk, iucv_sock_recvmsg() does:

		consume_skb(skb);
		if (iucv->transport == AF_IUCV_TRANS_HIPER) {
			atomic_inc(&iucv->msg_recv);
			if (atomic_read(&iucv->msg_recv) > iucv->msglimit) {
				WARN_ON(1);
				iucv_sock_close(sk);
				return -EFAULT;
			}
		}

The only place msg_recv is decremented is afiucv_hs_send() with
atomic_sub(confirm_recv, &iucv->msg_recv), reached through the
iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN) call in this hunk, and that call sits
under:

		if (skb_queue_empty(&iucv->backlog_skb_q)) {

Meanwhile afiucv_hs_callback_rx() queues every accepted frame without
enforcing the window it advertised to the peer:

net/iucv/af_iucv.c:afiucv_hs_callback_rx() {
	spin_lock(&iucv->message_q.lock);
	if (skb_queue_empty(&iucv->backlog_skb_q)) {
		if (__sock_queue_rcv_skb(sk, skb))
			skb_queue_tail(&iucv->backlog_skb_q, skb);
	} else
		skb_queue_tail(&iucv_sk(sk)->backlog_skb_q, skb);

So if a non-conforming peer floods the socket, the receive queue hits
sk_rcvbuf, __sock_queue_rcv_skb() keeps failing in the drain loop above,
the skb is re-queued at the head of backlog_skb_q and the loop breaks.
backlog_skb_q then stays non-empty, no window update is sent, and msg_recv
keeps incrementing on each recvmsg() until it passes msglimit
(IUCV_HIPER_MSGLIM_DEFAULT is 128) and the WARN fires.  With panic_on_warn
that would be a panic driven by a remote peer.

The same WARN also looks reachable locally with SO_MSGLIMIT set to 1 and
two concurrent recvmsg() threads, since recvmsg() does not hold the socket
lock across the atomic_inc.

Would a rate-limited message plus a drop or reset be a better response to a
peer protocol violation here than WARN_ON(1)?
-- 
pw-bot: cr