Re: [PATCH net v2] net/iucv: descend into net/iucv independently of IUCV

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

New issues:
- [High] Unconditional descent into net/iucv/ causes a vmlinux linker error when CONFIG_AFIUCV=y and CONFIG_IUCV=m.

Pre-existing issues:
- [High] Packet handler modifies and queues shared SKBs without ensuring exclusive ownership, causing list corruption and data corruption.
--

--- Patch [1]: [PATCH net v2] net/iucv: descend into net/iucv independently of IUCV ---
commit e7b1f87fb6bb063631fc940969ffda9c69434171
Author: Pengpeng Hou <[email protected]>

net/iucv: descend into net/iucv independently of IUCV

AFIUCV can be enabled as either built-in or a module through QETH_L3 even
when IUCV is disabled. The top-level net Makefile currently descends into
net/iucv/ only under CONFIG_IUCV, so the valid configurations
CONFIG_AFIUCV=y/m with CONFIG_IUCV=n never consider af_iucv.o.

> diff --git a/net/Makefile b/net/Makefile
> index 5b2dd7f07a85e..c0da946b281e1 100644
> --- a/net/Makefile
> +++ b/net/Makefile
> @@ -45,7 +45,7 @@ obj-$(CONFIG_WIRELESS)		+= wireless/
>  obj-$(CONFIG_MAC80211)		+= mac80211/
>  obj-$(CONFIG_TIPC)		+= tipc/
>  obj-$(CONFIG_NETLABEL)		+= netlabel/
> -obj-$(CONFIG_IUCV)		+= iucv/
> +obj-y				+= iucv/

[Severity: High]
Does unconditionally descending into iucv/ cause a vmlinux linker error when
CONFIG_AFIUCV=y and CONFIG_IUCV=m?

Before this patch, Kbuild descended into net/iucv/ as a module (obj-m),
compiling af_iucv.o as a module. With this change, af_iucv.o is compiled
into vmlinux when CONFIG_AFIUCV=y.

However, af_iucv.c contains:

net/iucv/af_iucv.c:afiucv_init() {
    ...
    if (machine_is_vm() && IS_ENABLED(CONFIG_IUCV)) {
        cpcmd("QUERY USERID", iucv_userid, sizeof(iucv_userid), &err);
        ...
        pr_iucv = &iucv_if;
    }

Because IS_ENABLED(CONFIG_IUCV) evaluates to 1 when CONFIG_IUCV=m, the
built-in afiucv_init() will emit an external reference to iucv_if (which
resides in the iucv.ko module), leading to an undefined reference linker
error.

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the IUCV
code, I noticed a pre-existing issue in the AF_IUCV packet handlers.

Could the packet handler corrupt memory by modifying and queuing shared SKBs
without ensuring exclusive ownership?

When a HiperSockets AF_IUCV packet is received by afiucv_hs_rcv() while a
packet socket like tcpdump is concurrently active on the same interface,
the network core passes a shared or cloned SKB to the handlers.

afiucv_hs_rcv() and its callbacks modify the packet data in-place without
calling skb_share_check() or skb_unshare():

net/iucv/af_iucv.c:afiucv_hs_rcv() {
    ...
    EBCASC(trans_hdr->destAppName, sizeof(trans_hdr->destAppName));
    EBCASC(trans_hdr->destUserID, sizeof(trans_hdr->destUserID));
    ...
}

The same happens in the callbacks before transmission:

net/iucv/af_iucv.c:afiucv_hs_callback_syn() {
    ...
    skb_push(skb, ETH_HLEN);
    memset(skb->data, 0, ETH_HLEN);
    ...
}

Furthermore, it enqueues the shared SKB directly into the backlog:

net/iucv/af_iucv.c:afiucv_hs_callback_rx() {
    ...
    skb_queue_tail(&iucv->backlog_skb_q, skb);
    ...
}

If the packet socket also enqueues the SKB, the SKB's next and prev
pointers are concurrently overwritten, which can lead to list corruption
and a kernel panic.

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