Re: [PATCH net v2] s390/ctcm: Prevent XID null dereference

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Integer underflow when parsing XID buffer length in `mpc_validate_xid()` leads to bounds check bypass and an erroneously large maximum buffer size, causing `skb_over_panic` DoS.
- [High] Calling kfree() on `priv->xid` causes slab allocator corruption because it is an interior pointer to an SKB data buffer.
- [High] Active timers are not safely canceled during device removal, leading to Use-After-Free.
- [High] The MPC group finite state machine (grp->fsm) and its associated state are accessed and modified concurrently from multiple contexts without any locking, leading to data races.
- [Medium] The `ch->xid_skb` buffer is leaked during MPC device removal/shutdown.
- [Medium] Copying uninitialized channel receive buffer memory into a new SKB when receiving short packets.
--

commit a7c53ce617cb2637fa08d34df71dac23170822fd
Author: Aswin Karuvally <aswin@linux.ibm.com>

s390/ctcm: Prevent XID null dereference

This commit fixes a NULL pointer dereference in mpc_validate_xid() by
using priv->xid instead of grp->saved_xid2 for error signaling.

> diff --git a/drivers/s390/net/ctcm_mpc.c b/drivers/s390/net/ctcm_mpc.c
> index 08e36685e5788..61c88fe853c55 100644
> --- a/drivers/s390/net/ctcm_mpc.c
> +++ b/drivers/s390/net/ctcm_mpc.c
[ ... ]
> @@ -1636,7 +1636,6 @@ static int mpc_validate_xid(struct mpcg_info *mpcginfo)
>  			"The XID used in the MPC protocol is not valid, "
>  			"rc = %d\n", rc);
>  		priv->xid->xid2_flag2 = 0x40;

[Severity: High]
This is a pre-existing issue, but I noticed this lockless write to
priv->xid->xid2_flag2. Since FSM events for the group (grp->fsm) are
dispatched via the lockless fsm_event() function and can be triggered
concurrently from different tasklets or timer softirqs, can this
modification race with other contexts reading or writing this state?

A similar pattern occurs when grp->outstanding_xid7 is decremented
locklessly in mpc_action_rcvd_xid7().

> -		grp->saved_xid2->xid2_flag2 = 0x40;
>  	}
>  
>  	return rc;

While reviewing this subsystem, I also noticed a few other pre-existing
issues in the surrounding code:


drivers/s390/net/ctcm_mpc.c:mpc_validate_xid() {
    ...
    if (xid->xid2_dlc_type == XID2_READ_SIDE) {
        ...
        if (grp->group_max_buflen == 0 || grp->group_max_buflen >
                        xid->xid2_buf_len - len)
            grp->group_max_buflen = xid->xid2_buf_len - len;
    }
    ...
}

[Severity: Critical]
This isn't a bug introduced by this patch, but does this buffer length
calculation suffer from an integer underflow?

The len variable is an int evaluated to 32. If a peer sends a small
xid2_buf_len (e.g., 16), the subtraction evaluates to a negative int.
Since grp->group_max_buflen is a __u16, the comparison bypasses the
bounds check, and the assignment casts the negative integer to a large
positive value (e.g., 65520). 

Could this cause an skb_over_panic later in ctcmpc_chx_txdone() by
enqueuing more data than the allocated SKB can hold?


drivers/s390/net/ctcm_main.c:ctcm_free_netdevice() {
    ...
        if (grp->xid_skb)
            dev_kfree_skb(grp->xid_skb);
    ...
    if (priv->xid) {
        kfree(priv->xid);
        priv->xid = NULL;
    }
}

[Severity: High]
This is a pre-existing issue, but does calling kfree() on priv->xid
cause slab allocator corruption? 

In ctcmpc_init_mpc_group(), grp->xid is initialized via
skb_tail_pointer(grp->xid_skb) which is an interior offset within the
SKB's data buffer, and priv->xid is assigned this value. Passing a
pointer that aliases the interior of an already-freed SKB to kfree()
appears to be an invalid free and a use-after-free.


drivers/s390/net/ctcm_main.c:channel_remove() {
    ...
        fsm_deltimer(&ch->timer);
        if (IS_MPC(ch))
            fsm_deltimer(&ch->sweep_timer);

        kfree_fsm(ch->fsm);
    ...
        kfree(ch);
}

[Severity: High]
This isn't a bug introduced by this patch, but are active timers safely
canceled here during device removal? 

The fsm_deltimer() routine uses an asynchronous timer_delete(). If the
timer handler is executing concurrently on another CPU, would immediately
freeing the fsm and ch structures lead to a use-after-free? 

Similarly, in ctcm_free_netdevice(), grp is freed without canceling
grp->timer.


drivers/s390/net/ctcm_main.c:channel_remove() {
    ...
        if (IS_MPC(ch)) {
            tasklet_kill(&ch->ch_tasklet);
            tasklet_kill(&ch->ch_disc_tasklet);
            kfree(ch->discontact_th);
        }
        kfree(ch->ccw);
        kfree(ch->irb);
        kfree(ch);
}

[Severity: Medium]
This is a pre-existing issue, but does this teardown path leak the
ch->xid_skb buffer? 

For MPC devices, ctcm_shutdown_device() calls ctcm_close(), which is a
no-op, and channel_free() does not trigger DEV_EVENT_RXDOWN. Because the
event isn't sent to the device FSM, mpc_channel_action() isn't called to
properly free the MPC structures, and channel_remove() appears to leave
the SKB leaked.


drivers/s390/net/ctcm_fsms.c:mpc_action_dopacket() {
    ...
    default:
        len = TH_HEADER_LENGTH + XID2_LENGTH + 4;
        ...
        skb_put_data(new_skb, skb->data, len);
}

[Severity: Medium]
This isn't a bug introduced by this patch, but does unconditionally
copying ~270 bytes into the new SKB cause an out-of-bounds read when
receiving short packets? 

If the actual received length from the CCW is shorter, this would copy
uninitialized channel receive buffer memory or stale data from previous
packets into the new SKB.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827063408.2168914-1-aswin@linux.ibm.com?part=1