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

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

Pre-existing issues:
- [High] Calling a sleeping memory allocation function (`sock_alloc_send_skb`) while holding a spinlock (`spin_lock_bh`) causes a sleep-in-atomic bug.
- [High] Concurrent `recvmsg` calls can trigger a Use-After-Free or NULL pointer dereference of the network device.
- [High] Iterating over the global socket list `iucv_sk_list` without holding the required read lock leads to list corruption and data races.
--

--- Patch [1]: [PATCH net 1/2] net/iucv: only send the window update on HiperSockets sockets ---
commit 8751dd7a500553e40dc5fbd7b9476a646ff45fb5
Author: Bryam Vargas <[email protected]>

net/iucv: only send the window update on HiperSockets sockets

> 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.
> 
> Test the transport. Three of the other four iucv_send_ctrl() callers do;
> the fourth, afiucv_netdev_event(), selects on hs_dev instead. Without
> relocate_lowcore the NULL read lands in mapped lowcore and the symptom is
> the spurious disconnect; with lowcore relocation it faults.

[Severity: High]
This is a pre-existing issue, but does iterating over the global socket list
iucv_sk_list locklessly in afiucv_netdev_event() lead to list corruption and
data races?

afiucv_netdev_event() traverses the iucv_sk_list.head list using sk_for_each()
while holding only the RTNL lock:

net/iucv/af_iucv.c:afiucv_netdev_event() {
	...
	sk_for_each(sk, &iucv_sk_list.head) {
		iucv = iucv_sk(sk);
		if ((iucv->hs_dev == event_dev) &&
	...
}

However, sockets can be created (via iucv_sock_bind() -> iucv_sock_link()) or
destroyed (via iucv_sock_kill() -> iucv_sock_unlink()) concurrently. These
modify the list while holding write_lock_bh(&iucv_sk_list.lock), but NOT the
RTNL lock.

Iterating a mutated hlist locklessly without acquiring
read_lock(&iucv_sk_list.lock) or rcu_read_lock() could violate locking
invariants and lead to list corruption, infinite loops, or a Use-After-Free.

> 
> Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
> Cc: [email protected]
> Signed-off-by: Bryam Vargas <[email protected]>
> 
> 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);

[Severity: High]
This isn't a bug introduced by this patch, but doesn't calling
iucv_send_ctrl() here cause a sleep-in-atomic bug?

Since iucv_sock_recvmsg() is holding spin_lock_bh(&iucv->message_q.lock):

net/iucv/af_iucv.c:iucv_sock_recvmsg() {
	...
	spin_lock_bh(&iucv->message_q.lock);
	...
	if (iucv->transport == AF_IUCV_TRANS_HIPER && ...) {
		err = iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN);
	...
}

And iucv_send_ctrl() calls sock_alloc_send_skb():

net/iucv/af_iucv.c:iucv_send_ctrl() {
	...
	skb = sock_alloc_send_skb(sk, blen, 1, &err);
	...
}

Which can sleep because sk->sk_allocation defaults to GFP_KERNEL (retaining
__GFP_DIRECT_RECLAIM).

Could this trigger a kernel panic from might_sleep() or a soft lockup?


[Severity: High]
This is also a pre-existing issue, but could concurrent recvmsg calls trigger
a Use-After-Free or NULL pointer dereference of the network device?

sys_recvmsg() can be called concurrently by multiple threads on the same
socket since iucv_sock_recvmsg() does not hold the socket lock.

If Thread A reads enough messages to hit
atomic_read(&iucv->msg_recv) > iucv->msglimit, it calls iucv_sock_close(sk).
This acquires the socket lock, drops the reference to iucv->hs_dev, and sets
it to NULL:

net/iucv/af_iucv.c:iucv_sock_close() {
	...
	dev_put(iucv->hs_dev);
	iucv->hs_dev = NULL;
	...
}

Concurrently, if Thread B hits the check below and calls iucv_send_ctrl():

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

It reads and dereferences iucv->hs_dev locklessly, which could result in a UAF
or NULL pointer dereference.

>  				if (err) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1