* Re: [PATCH net] s390/ctcm: Prevent XID null dereference
2026-08-09 10:55 [PATCH net] s390/ctcm: Prevent XID null dereference Aswin Karuvally
@ 2026-08-10 10:56 ` sashiko-bot
2026-08-17 17:41 ` Jakub Kicinski
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-08-10 10:56 UTC (permalink / raw)
To: Aswin Karuvally
Cc: Alexander Gordeev, Heiko Carstens, Vasily Gorbik, linux-s390
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 <aswin@linux.ibm.com>
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: stable@vger.kernel.org
> 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/20260809105536.541453-1-aswin@linux.ibm.com?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net] s390/ctcm: Prevent XID null dereference
2026-08-09 10:55 [PATCH net] s390/ctcm: Prevent XID null dereference Aswin Karuvally
2026-08-10 10:56 ` sashiko-bot
@ 2026-08-17 17:41 ` Jakub Kicinski
1 sibling, 0 replies; 3+ messages in thread
From: Jakub Kicinski @ 2026-08-17 17:41 UTC (permalink / raw)
To: aswin
Cc: Jakub Kicinski, davem, pabeni, edumazet, andrew+netdev, netdev,
linux-s390, hca, gor, agordeev, wintera, borntraeger, svens,
horms, jeff, stable
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
^ permalink raw reply [flat|nested] 3+ messages in thread