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

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

New issues:
- [High] The XID validation logic creates an asymmetric error handling flaw where a failure on the first processed channel is silently ignored if the second channel succeeds, allowing the group to incorrectly transition to the READY state.

Pre-existing issues:
- [High] Calling `kfree()` on an interior pointer to a previously freed sk_buff data buffer during device teardown.
- [High] An integer underflow when calculating `group_max_buflen` from untrusted network input can lead to a massively oversized buffer limit, potentially causing an `skb_put` panic during transmission.
- [High] Device teardown frees core structures without synchronously cancelling FSM timers, leading to a Use-After-Free.
- [High] Group FSM state is accessed concurrently without locks, leading to data races on shared variables.
--

--- Patch [1]: [PATCH net] s390/ctcm: Prevent XID null dereference ---
commit eba778bf6c3c6c80501b99041bb51b27ad5c49b6
Author: Aswin Karuvally <[email protected]>
Subject: s390/ctcm: Prevent XID null dereference

The mpc_validate_xid() function sets grp->saved_xid2->xid2_flag2 to 0x40
to signal XID validation error. This commit updates it to prevent a null
dereference if the peer XID is NULL or an r/w channel pairing mismatch
occurs. It also updates mpc_action_go_ready() to handle the uninitialized
structure.

Cc: [email protected]

> diff --git a/drivers/s390/net/ctcm_mpc.c b/drivers/s390/net/ctcm_mpc.c
> --- a/drivers/s390/net/ctcm_mpc.c
> +++ b/drivers/s390/net/ctcm_mpc.c
> @@ -826,7 +826,7 @@ static void mpc_action_go_ready(fsm_instance *fsm, int event, void *arg)
>  
>  	fsm_deltimer(&grp->timer);

[Severity: High]
This is a pre-existing issue, but I am curious about the teardown sequence
when cancelling timers here and during interface removal.

fsm_deltimer() wraps timer_delete(), which doesn't wait for concurrent
timer callbacks to finish.

If a timer expires concurrently during device teardown (ctc_mpc_dealloc_ch)
while the driver proceeds to free grp and grp->fsm, could the executing
timer callback dereference freed memory when it calls fsm_event()?

> -	if (grp->saved_xid2->xid2_flag2 == 0x40) {
> +	if (!grp->saved_xid2 || grp->saved_xid2->xid2_flag2 == 0x40) {

[Severity: High]
If the first channel fails validation (e.g., mismatched direction) and sets
priv->xid->xid2_flag2 to 0x40, grp->saved_xid2 remains NULL.

If the second channel then succeeds, it initializes grp->saved_xid2 with a
valid xid where xid2_flag2 is 0x00. 

Since this check only evaluates grp->saved_xid2->xid2_flag2, could this
allow the state machine to falsely bypass the check and transition to the
ready state, completely ignoring the earlier failure stored in
priv->xid->xid2_flag2?


[Severity: High]
This is a pre-existing issue, but does this state machine implementation
have data races on shared group state variables?

The read and write channels are separate ccw_device instances, so their
irq handlers use different locks. Because fsm_event() dispatches actions
without internal synchronization:

fsm_event()
    state = atomic_read(&fi->state);
    // executes action directly

Can concurrent irq handlers from different channels execute read-modify-write
operations on shared fields (like grp->outstanding_xid2--) simultaneously,
corrupting the state?

>  		priv->xid->xid2_flag2 = 0x00;
>  		if (grp->estconnfunc) {

[ ... ]

> @@ -1636,7 +1636,13 @@ static int mpc_validate_xid(struct mpcg_info *mpcginfo)

[Severity: High]
This is a pre-existing issue, but could the buffer calculation earlier in
mpc_validate_xid() underflow?

mpc_validate_xid() receives an untrusted xid2_buf_len from the network:

    if (grp->group_max_buflen == 0 || grp->group_max_buflen > xid->xid2_buf_len - len)
        grp->group_max_buflen = xid->xid2_buf_len - len;
    
If xid->xid2_buf_len is less than len, does the subtraction underflow to a
huge value?

Since data_space is calculated as grp->group_max_buflen - TH_HEADER_LENGTH
in ctcmpc_chx_txdone(), and skb_put_data() relies on it, can this trigger
an skb_over_panic when packing data into the transmit sk_buff?

>  			"The XID used in the MPC protocol is not valid, "
>  			"rc = %d\n", rc);
>  		priv->xid->xid2_flag2 = 0x40;
> -		grp->saved_xid2->xid2_flag2 = 0x40;
> +
> +		/* If xid is NULL (rc=1) or r/w channel pairing mismatch
> +		 * (rc=2) happens, grp->saved_xid2 is never initialized
> +		 * and becomes NULL
> +		 */
> +		if (grp->saved_xid2)
> +			grp->saved_xid2->xid2_flag2 = 0x40;
>  	}
>  
>  	return rc;

[Severity: High]
This is a pre-existing issue, but I wanted to ask about the lifecycle of
these pointers during teardown. 
    
When the group is initialized:

ctcmpc_init_mpc_group()
    grp->xid = (struct xid2 *)skb_tail_pointer(grp->xid_skb);
    ...
    priv->xid = grp->xid;
    
During device teardown:

ctcm_free_netdevice()
    dev_kfree_skb(grp->xid_skb);
    ...
    kfree(priv->xid);

Does this sequence cause a use-after-free and an invalid free by passing an
interior pointer of the already freed sk_buff data buffer directly to
kfree()?

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