RE: [External Mail] Re: [PATCH v4 5/7] net: wwan: t9xx: Add FSM thread

["Wu. JackBB (GSM)" <JackBB_Wu@compal.com>]

Hi Simon,

> > +static int mtk_fsm_parse_hs2_msg(...)
>
> [Severity: High]
> Assumes entire SKB payload is linearly accessible without checking
> boundaries using pskb_may_pull() before direct dereferencing.

The RX SKBs are allocated internally by the CLDMA driver using
__dev_alloc_skb with linear buffers. The control message data
always resides in the contiguous linear data region. These SKBs
are not received from the network stack and are always linear,
so pskb_may_pull is not needed.

> [Severity: High]
> The rtft_entry->data is passed to action callbacks before data_len
> is validated against buffer bounds.

The bounds check at the loop start ensures the rtft_entry struct
header is within bounds. The action functions only read fixed-size
fields (e.g., a single __le32 for packet padding mode). The modem
firmware is a trusted source with head_pattern/tail_pattern
integrity checks.

> > +static int mtk_fsm_send_hs3_msg(...)
> > +	skb = __dev_alloc_skb(...)
>
> [Severity: High]
> Uninitialized kernel heap memory leak. Buffer is not zeroed.

In mtk_fsm_append_rtft_entries, each runtime_feature_entry has
feature_id, support_info, and data_len explicitly set. The
reserved[2] field is not zeroed, but this data is sent to the
local modem over PCIe DMA as part of the HS3 handshake — it is
not exposed to external networks. The modem firmware ignores the
reserved field.

> > +static int mtk_fsm_sap_ctrl_msg_handler(...)
> > +	skb_pull(skb, ...)
>
> [Severity: High]
> Missing pskb_may_pull() call.

Same as above. The RX SKBs are internal linear buffers from the
CLDMA driver. skb->len is checked before accessing skb->data.

> [Severity: High]
> Dangling pointer and SKB leak. If mtk_fsm_evt_submit() fails,
> SKB is freed but hs_info->rt_data not nulled.

We will add hs_info->rt_data = NULL after dev_kfree_skb(skb) in
the evt_submit failure path to avoid leaving a dangling pointer.
The same fix will be applied to mtk_fsm_md_ctrl_msg_handler
which has the identical pattern.

> > +static int mtk_port_ch_disable(...)
> > +	wait_event_interruptible_timeout(...)
>
> [Severity: High]
> Infinite busy loop on signal pending.

This is intentional. Channel disable must complete or timeout —
aborting mid-operation would leave the channel in an inconsistent
state. The loop does not busy-spin: each iteration sleeps in
wait_event_interruptible_timeout until the condition is met or
the timeout expires. The timeout guarantees bounded execution.

> > +static int mtk_fsm_evt_submit(...)
> > +	spin_unlock_irqrestore(...)
>
> [Severity: High]
> Race condition during teardown. fsm->fsm_handler could become
> NULL before wake_up_process().

mtk_fsm_exit() first submits a FSM_EVT_DEV_RM event. The FSM
kthread processes this and sets EVT_TF_GATECLOSED under
evtq_lock. Only after the kthread finishes does mtk_fsm_exit()
call kthread_stop() and set fsm_handler to NULL. If
mtk_fsm_evt_submit() passes the EVT_TF_GATECLOSED check inside
the spinlock, fsm_handler is still valid at the point of
wake_up_process().

> > +static int mtk_port_disable(...)
> > +	kcalloc(...)
>
> [Severity: High]
> Silent allocation failure.

The kcalloc allocates a small temporary array of pointers
(typically a few hundred bytes). Allocation failure at this size
is extremely unlikely. Additionally, mtk_port_disable is called
during FSM_STATE_OFF transition, when the modem hardware has
already been power-cycled via PLDR. The hardware channels are
no longer active regardless of the software port state.

> > +static int mtk_fsm_notifier_register(...)
>
> [Severity: Medium]
> Race condition on notifier lists.

The notifier registration and unregistration are only called
during driver probe and remove paths, which are serialized by
the driver model. The FSM kthread does not start processing
events until probe completes, so there is no concurrent access
to the notifier lists.

> > +static void mtk_fsm_evt_cleanup(...)
>
> [Severity: Medium]
> Incomplete event queue cleanup. Cleanup misses freeing SKB stored
> in shared fsm->hs_info[].rt_data if HS2 message was pending.
We Will add rt_data cleanup in the teardown path in v5.

Thanks.

Jack Wu