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

[Alexandra Winter]

On 28.08.26 21:07, Bryam Vargas via B4 Relay wrote:
> From: Bryam Vargas <hexlabsecurity@xxxxxxxxx>
> 
> iucv_sock_recvmsg() sends the HiperSockets-only AF_IUCV_FLAG_WIN without
> testing the transport, so on a classic z/VM socket iucv_send_ctrl() sizes
> the skb through a NULL iucv->hs_dev. SO_MSGLIMIT accepts 1, so msglimit / 2
> is zero and one recvmsg() on its own socket is enough for an unprivileged
> process to take a spurious disconnect.
> 
> It also calls iucv_send_ctrl() under spin_lock_bh(&message_q.lock), which
> allocates GFP_KERNEL inside a section the code treats as atomic. Sending
> outside that lock lets two recvmsg() reach afiucv_hs_send() at once, where
> msg_recv is sampled for the advertised window and subtracted after
> dev_queue_xmit() -- and sendmsg reaches that counter under lock_sock()
> while recvmsg holds no socket lock, so both can subtract the same value,
> the counter goes negative and the credit reaches the peer twice.
> 
> Test the transport, claim the credit with atomic_xchg() after the last
> error exit and hand it back if the transmit fails, and send once the lock
> is dropped.
> 
> Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
^^^ This is the older commit and should be used as Fixes-Tag
(alloc under spin lock and HiperSockets send in a z/VM scenario)> Fixes: 238965b71b96 ("net/af_iucv: build proper skbs for HiperTransport")
^^^
This is a newer commit and add the skb build consequence described above.
I think it needs to be drpped as Fixes tag.

I don't think you can have 2 Fixes tags, can you?


> Cc: stable@xxxxxxxxxxxxxxx
> Tested-by: Aswin Karuvally <aswin@xxxxxxxxxxxxx>
> Signed-off-by: Bryam Vargas <hexlabsecurity@xxxxxxxxx>
> ---
> v3: squashed the three v2 patches into one, as Alexandra Winter asked -- they
>     are one subject (iucv->msg_recv handling), they had a stated dependency,
>     and one patch is easier to backport. No code change from v2 1-3 combined.
> 
>     The v2 1/3 changelog said the NULL read faults under relocate_lowcore.
>     That is out: Alexandra booted with it and did not hit a page fault. I have
>     no explanation to offer in its place: vmem.c:521-524 skips the first
>     sizeof(struct lowcore) bytes from the identity mapping whether or not the
>     lowcore was relocated. So the changelog now claims only the disconnect,
>     which she reproduced.
> 
>     Tested-by carried from v2: the code is identical to v2 1-3 applied in
>     order.
> 
> v2: https://lore.kernel.org/all/20260821-b4-disp-3a6e8695-v2-0-37597ff723a8@xxxxxxxxx/
> v1: https://lore.kernel.org/all/20260815-b4-disp-8a791503-v1-0-fbae9a511144@xxxxxxxxx/
> 
> Not reproduced here: CONFIG_AFIUCV depends on S390 and I have no Z. The
> counter race was checked under LKMM -- the counter reaches -2 before the
> atomic_xchg() and cannot after -- and the tree builds clean for s390.
> ---

Reviewed-by: Alexandra Winter <wintera@xxxxxxxxxxxxx>

netdev maintainers, feel free to remove the second Fixes tag.

(Re-tested on s390)


>