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

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
s390/ctcm: Prevent XID null dereference

This patch adds a NULL check before writing the 0x40 validation-error flag
into grp->saved_xid2->xid2_flag2 in mpc_validate_xid(), and teaches
mpc_action_go_ready() to treat an uninitialized grp->saved_xid2 as a
validation error.

A few questions about the new guards and about the surrounding code they
interact with.

> diff --git a/drivers/s390/net/ctcm_mpc.c b/drivers/s390/net/ctcm_mpc.c
> index aeb102537e7f..a5b55fe6b110 100644
> --- 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);
>  
> -	if (grp->saved_xid2->xid2_flag2 == 0x40) {
> +	if (!grp->saved_xid2 || grp->saved_xid2->xid2_flag2 == 0x40) {

[Severity: Medium]
This is a pre-existing issue, but can the new check still be defeated by a
concurrent writer?  The condition performs two separate loads of
grp->saved_xid2 with no lock, READ_ONCE() or barrier:

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

grp->saved_xid2 is stored to NULL by mpc_action_go_inop() without any lock
held, and that path is reachable from ctcm_irq_handler() in hardirq context
via the channel FSM actions in ctcm_fsms.c that raise MPCG_EVENT_INOP, as
well as from the FSM timers.  If the NULL store lands between the two
loads, the guarded dereference still faults.  The only locks in this file
are ch->collect_lock and the ccw device lock, neither of which covers the
group state.

Note also that fsm_deltimer() just calls timer_delete():

drivers/s390/net/fsm.c:fsm_deltimer() {
	timer_delete(&this->tl);
}

so the fsm_deltimer(&grp->timer) above does not wait for an already-running
mpc_action_timeout() -> MPCG_EVENT_INOP callback on another CPU.  Would a
snapshot load, or real group-level locking with spin_lock_irqsave() since
one writer runs in hardirq, be more appropriate here?

>  		priv->xid->xid2_flag2 = 0x00;
>  		if (grp->estconnfunc) {
>  			grp->estconnfunc(grp->port_num, 1,
> @@ -1636,7 +1636,13 @@ 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;
> -		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;

[Severity: High]
Does skipping this store lose the rejection entirely?

grp->saved_xid2->xid2_flag2 looks like the only place the MPC group FSM
records a failed validation.  The other candidates do not seem to be read
anywhere:

  - both callers, mpc_action_rcvd_xid0() and mpc_action_rcvd_xid7(),
    discard the return value of mpc_validate_xid() and advance the FSM on
    the outstanding-XID counters alone
  - priv->xid->xid2_flag2 is only written, never tested
  - grp->xidnogood is only ever assigned 0 and never tested

So when the first XID for a group is rejected with rc = 2 (r/w channel
pairing mismatch), nothing is latched, and the next XID takes the
initialization branch:

	if (grp->saved_xid2 == NULL) {
		grp->saved_xid2 =
			(struct xid2 *)skb_tail_pointer(grp->rcvd_xid_skb);

		skb_put_data(grp->rcvd_xid_skb, xid, XID2_LENGTH);

which installs the peer record with xid2_flag2 == 0.  At that point the new
!grp->saved_xid2 test in mpc_action_go_ready() no longer fires and the
group is driven to MPCG_STATE_READY despite the earlier rejection.

The rc = 2 early exit also skips the buffer-length negotiation:

	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;

so a peer that first sends an XID with xid2_dlc_type == XID2_WRITE_SIDE on
our write channel (rc = 2, nothing latched, buflen not updated) and then a
well-formed XID2_WRITE_SIDE XID on our read channel gets to READY with
grp->group_max_buflen == 0.  ctcmpc_chx_txdone() then computes the tx copy
budget into a __u32:

drivers/s390/net/ctcm_fsms.c:ctcmpc_chx_txdone() {
	__u32		data_space;
	...
	data_space = grp->group_max_buflen - TH_HEADER_LENGTH;

which underflows to 0xFFFFFFF8, so the collect_queue drain loop keeps
calling skb_put_data() into ch->trans_skb with no effective bound and can
reach skb_over_panic().

Secondary effect: because mpc_action_go_ready() takes the success branch,
priv->xid->xid2_flag2 is never reset to 0x00, and since priv->xid aliases
grp->xid inside grp->xid_skb, which mpc_channel_action() copies into every
channel XID:

		skb_put_data(ch->xid_skb, grp->xid_skb->data,
			     grp->xid_skb->len);

later negotiations keep advertising NOGOOD.  Pre-patch this sequence oopsed
on the NULL dereference instead, so reaching READY after a rejected XID is
newly reachable.  Would latching the rejection, for example in
grp->xidnogood or by forcing MPCG_EVENT_INOP, be preferable to dropping it?

[Severity: Low]
This isn't a bug, but the new comment states an invariant the code does not
have.  "grp->saved_xid2 is never initialized and becomes NULL" reads as if
the rc = 1 and rc = 2 paths clear the pointer.  The pointer is sticky
instead: the only non-NULL assignment is guarded by
if (grp->saved_xid2 == NULL), and it is reset to NULL only by the group
init/inop/alloc/establish paths.  An rc = 2 rejection that happens after an
earlier XID was accepted therefore runs with grp->saved_xid2 non-NULL.

The rc = 1 case named in the comment also does not look reachable.
ctcmpc_unpack_skb() sets mpcginfo->xid unconditionally right before raising
the event:

			mpcginfo->xid = thisxid;
			fsm_event(grp->fsm, MPCG_EVENT_XID2, mpcginfo);

and MPCG_EVENT_XID2 is the only event dispatching
mpc_action_rcvd_xid0()/mpc_action_rcvd_xid7(), the only callers of
mpc_validate_xid().  Could the comment be reworded to say simply that
grp->saved_xid2 may not have been initialized yet?

The same wording appears in the commit message: "If peer XID is NULL or r/w
channel pairing mismatch happens, grp->saved_xid2 is never initialized."

[Severity: High]
This isn't introduced by this patch, but it sits in the function being
modified: is xid->xid2_buf_len validated anywhere before it becomes the tx
copy budget?  In mpc_validate_xid():

	int	len = TH_HEADER_LENGTH + PDU_HEADER_LENGTH;
	...
		if (grp->group_max_buflen == 0 || grp->group_max_buflen >
						xid->xid2_buf_len - len)
			grp->group_max_buflen = xid->xid2_buf_len - len;

The subtraction is done in int and stored into the __u16
grp->group_max_buflen, so a peer advertising xid2_buf_len == 0 yields
65524, and xid2_buf_len == 12 yields exactly 0, which the "== 0" test then
treats as "not negotiated" forever.  There is no clamp against
ch->max_bufsize either.

group_max_buflen == 0 then underflows data_space in ctcmpc_chx_txdone() as
described above, and the MPC tx path queues without a length cap:

drivers/s390/net/ctcm_main.c:ctcmpc_transmit_skb() {
		skb_queue_tail(&ch->collect_queue, skb);
		ch->collect_len += skb->len;

unlike ctcm_transmit_skb(), which checks collect_len against
max_bufsize - 2.  Should mpc_validate_xid() reject xid2_buf_len <= len and
clamp the result to ch->max_bufsize?

[Severity: Medium]
This is also pre-existing, but in the same function the shared pointer is
published before the memory it points at is initialized:

	if (grp->saved_xid2 == NULL) {
		grp->saved_xid2 =
			(struct xid2 *)skb_tail_pointer(grp->rcvd_xid_skb);

		skb_put_data(grp->rcvd_xid_skb, xid, XID2_LENGTH);
		grp->rcvd_xid_skb->data = grp->rcvd_xid_data;

mpc_validate_xid() runs from the read-channel and write-channel tasklets
(separate ch->ch_tasklet instances for the same grp), so can both CPUs see
grp->saved_xid2 == NULL, both publish, and both skb_put_data() into
grp->rcvd_xid_skb while resetting its data/tail/len?  The reader in the
else branch dereferences the pointer with no lock or acquire barrier:

		if (xid->xid2_flag4 != grp->saved_xid2->xid2_flag4) {

so it can compare against a published-but-uninitialized record.  Adding a
NULL check does not make this access safe on its own.

>  	}
>  
>  	return rc;

[Severity: High]
This isn't a bug introduced by this patch, but since the patch writes
through priv->xid in both hunks, the lifetime of that pointer looks wrong.
ctcm_free_netdevice() in drivers/s390/net/ctcm_main.c does:

		dev_kfree_skb(grp->xid_skb);
		dev_kfree_skb(grp->rcvd_xid_skb);
		...
		kfree(priv->xid);
		priv->xid = NULL;

but priv->xid is never a standalone allocation.  The only assignment is in
ctcmpc_init_mpc_group():

	grp->xid = (struct xid2 *)skb_tail_pointer(grp->xid_skb);
	...
	priv->xid = grp->xid;

taken after a TH header was put, so it is an interior pointer into
grp->xid_skb's data buffer, and that skb was already released by
dev_kfree_skb(grp->xid_skb) a few lines earlier.  Is the kfree() therefore
both a use-after-free and an invalid free of a non-allocation-start address?
grp->xid_skb comes from __dev_alloc_skb(MPC_BUFSIZE_DEFAULT), i.e. a large
kmalloc, so kfree() on head plus an offset would try to release the folio
again.  Reachable from ctcm_remove_device() -> unregister_netdev() ->
ctcm_free_netdevice() and from the ctcm_new_device() error path on any MPC
device.
-- 
pw-bot: cr