Re: [PATCH net 1/2] net/iucv: clear hs_dev before dropping its reference

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (3) · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In afiucv_hs_callback_syn(), the child now takes a reference
  on skb->dev every time, instead of copying the listener's hs_dev.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  afiucv_netdev_event() walks iucv_sk_list with sk_for_each() and holds…
- [High] This problem wasn't introduced by this patch, but
  afiucv_hs_callback_syn() leaks the child socket it allocates on every…
- [High] This problem wasn't introduced by this patch, but the SYN
  callback can publish a child into a listener's accept_q while that…

--

net/iucv: clear hs_dev before dropping its reference

This patch changes iucv_sock_close() so that iucv->hs_dev is cleared
before dev_put(). It also marks the racing hs_dev and sk_bound_dev_if
accesses with READ_ONCE()/WRITE_ONCE(). Finally, afiucv_hs_callback_syn()
now takes the child's device reference and sk_bound_dev_if from skb->dev
instead of from the listener's fields.

Closes: https://lore.kernel.org/netdev/b3fea2ca-25ff-42c0-812c-f8cc6bb6667e@linux.ibm.com/

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index db261ecd19af1..501e758c5e68a 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -447,9 +447,16 @@ static void iucv_sock_close(struct sock *sk)
>  	}
>  
>  	if (iucv->hs_dev) {
> -		dev_put(iucv->hs_dev);
> -		iucv->hs_dev = NULL;
> -		sk->sk_bound_dev_if = 0;
> +		struct net_device *dev = iucv->hs_dev;
> +
> +		/* afiucv_hs_rcv() matches frames on hs_dev under
> +		 * iucv_sk_list.lock, not the socket lock: clear it before the
> +		 * reference goes, so the match can no longer find a device
> +		 * this socket does not hold.
> +		 */
> +		WRITE_ONCE(iucv->hs_dev, NULL);
> +		WRITE_ONCE(sk->sk_bound_dev_if, 0);
> +		dev_put(dev);
>  	}

[Severity: High]
This is a pre-existing issue, but afiucv_netdev_event() also reads this
field, and it holds neither iucv_sk_list.lock nor the socket lock:

net/iucv/af_iucv.c:afiucv_netdev_event() {
    ...
	case NETDEV_GOING_DOWN:
		sk_for_each(sk, &iucv_sk_list.head) {
			iucv = iucv_sk(sk);
			if ((iucv->hs_dev == event_dev) &&
			    (sk->sk_state == IUCV_CONNECTED)) {
				if (event == NETDEV_GOING_DOWN)
					iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);
    ...
}

iucv_sock_close() runs under lock_sock() only. Can it move the socket out
of IUCV_CONNECTED and run this clear after the notifier's check but before
its call to iucv_send_ctrl()?

iucv_send_ctrl() reads hs_dev again and uses it without a NULL check:

	blen = sizeof(struct af_iucv_trans_hdr) +
	       LL_RESERVED_SPACE(iucv->hs_dev);

The list walk itself also looks unprotected. iucv_sock_link() and
iucv_sock_unlink() change the list under write_lock_bh(&iucv_sk_list.lock),
and rtnl does not exclude them.

Could a concurrent close() followed by iucv_sock_kill() unlink and free
the node the notifier is on?

Once this patch is applied, the notifier is the only unlocked,
unannotated reader of hs_dev left. The notes below the commit message
leave it to Nagamani PV's "net/iucv: fix races in afiucv_netdev_event()".

[ ... ]

> @@ -1912,9 +1919,14 @@ static int afiucv_hs_callback_syn(struct sock *sk, struct sk_buff *skb)
>  	memcpy(niucv->dst_user_id, trans_hdr->srcUserID, 8);
>  	memcpy(niucv->src_name, iucv->src_name, 8);
>  	memcpy(niucv->src_user_id, iucv->src_user_id, 8);
> -	nsk->sk_bound_dev_if = sk->sk_bound_dev_if;
> -	niucv->hs_dev = iucv->hs_dev;
> -	dev_hold(niucv->hs_dev);
> +	/* skb->dev is the device the lookup matched against the listener's
> +	 * hs_dev; the listener's own fields are cleared by close() under
> +	 * lock_sock(), which this softirq does not hold. nsk is in
> +	 * iucv_sk_list already: hold, then publish.
> +	 */
> +	WRITE_ONCE(nsk->sk_bound_dev_if, skb->dev->ifindex);
> +	dev_hold(skb->dev);
> +	WRITE_ONCE(niucv->hs_dev, skb->dev);

[Severity: Medium]
Can this leak a reference on skb->dev when the SYN races with close() of
the listener?

The callback holds only bh_lock_sock(sk). That does not exclude
iucv_sock_close() of the listener, which holds lock_sock(), and the
callback never checks sock_owned_by_user(). This sequence looks possible:

CPU1 (softirq)
afiucv_hs_callback_syn()
  bh_lock_sock(sk);
  sk->sk_state == IUCV_LISTEN, check passes

CPU2 (process context)
iucv_sock_close(listener)
  iucv_sock_cleanup_listen()
    drains accept_q
  WRITE_ONCE(iucv->hs_dev, NULL);
  dev_put(dev);

CPU1
  dev_hold(skb->dev);
  WRITE_ONCE(niucv->hs_dev, skb->dev);
  dev_queue_xmit(skb);
  iucv_accept_enqueue(sk, nsk);
  nsk->sk_state = IUCV_CONNECTED;

The child then sits in IUCV_CONNECTED on the accept_q of a listener that
has already been drained. Nothing accepts, closes or kills it.

Before this patch, the callback read the listener's hs_dev as NULL in
this interleaving, so dev_hold(NULL) did nothing and only the struct sock
leaked. Now the child holds the ingress device.

In the other interleaving, where hs_dev was read before the clear, the
old code already leaked the listener's device reference. This change
extends the device leak to the case that used to leak only the sock.

afiucv_netdev_event() does nothing on NETDEV_UNREGISTER. On
NETDEV_GOING_DOWN it only moves the child to IUCV_DISCONN.

Would unregister_netdevice() of that HiperSockets device then wait
forever in netdev_wait_allrefs()?

The notes below the commit message mention this orphaned child, and
that it now holds the device the frame arrived on.

>  	afiucv_swap_src_dest(skb);
>  	trans_hdr->flags = AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_ACK;
>  	trans_hdr->window = niucv->msglimit;

[Severity: High]
This isn't a bug introduced by this patch, but does
afiucv_hs_callback_syn() leak the child socket nsk on every refused SYN?

iucv_sock_alloc() runs before any of the checks. It clears SOCK_ZAPPED
and links nsk into iucv_sk_list:

net/iucv/af_iucv.c:iucv_sock_alloc() {
    ...
	sock_reset_flag(sk, SOCK_ZAPPED);

	sk->sk_protocol = proto;
	sk->sk_state	= IUCV_OPEN;

	iucv_sock_link(&iucv_sk_list, sk);
    ...
}

Two branches call iucv_sock_kill(nsk): the refusal branch (listener not
in IUCV_LISTEN, or sk_acceptq_is_full()) and the dev_queue_xmit() failure
branch. iucv_sock_kill() returns at once for a socket that is not zapped:

net/iucv/af_iucv.c:iucv_sock_kill() {
	if (!sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket)
		return;
    ...
}

So nsk is never unlinked or freed. A HiperSockets peer can keep sending
SYNs to a bound socket, or to a listener with a full backlog. Each leaked
entry also makes the sk_for_each() walk in afiucv_hs_rcv() longer.

On the dev_queue_xmit() failure branch, the leaked child has already done
dev_hold(skb->dev) above.

Can that reference also block unregister_netdevice() of the ingress
device forever?

The notes below the commit message say Hidayath Khan reported this in
August and that his fix is pending.

[Severity: High]
This is a pre-existing issue, but can the child be published into the
listener's accept_q while the listener is being torn down, and then be
handled before it is fully set up?

The callback holds only bh_lock_sock(sk), while iucv_sock_close() holds
lock_sock(sk). iucv_accept_enqueue() makes the child visible before it
sets the child's parent:

net/iucv/af_iucv.c:iucv_accept_enqueue() {
    ...
	spin_lock_irqsave(&par->accept_q_lock, flags);
	list_add_tail(&iucv_sk(sk)->accept_q, &par->accept_q);
	spin_unlock_irqrestore(&par->accept_q_lock, flags);
	iucv_sk(sk)->parent = parent;
	sk_acceptq_added(parent);
}

iucv_sock_cleanup_listen() calls iucv_accept_dequeue(parent, NULL). That
function walks accept_q without accept_q_lock and, with newsock == NULL,
takes any child whatever its state. It then calls iucv_accept_unlink():

net/iucv/af_iucv.c:iucv_accept_unlink() {
    ...
	struct iucv_sock *par = iucv_sk(iucv_sk(sk)->parent);

	spin_lock_irqsave(&par->accept_q_lock, flags);
    ...
}

sk_alloc() zeroes the sock, so parent is still NULL if the store in
iucv_accept_enqueue() has not happened yet.

Is that a NULL pointer dereference?

If the parent store has happened, cleanup goes on to iucv_sock_close()
and iucv_sock_kill() the child, which drops both references. The callback
then still does:

		iucv_accept_enqueue(sk, nsk);
		nsk->sk_state = IUCV_CONNECTED;

Would the write to nsk->sk_state then be a use-after-free?

The commit message leaves the case of a SYN accepted while the listener
closes to the socket-locking rework.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022-b4-disp-b35a9153-v1-0-d74645876195%40proton.me