RE: [External Mail] Re: [PATCH v4 4/7] net: wwan: t9xx: Add control port

[Wu. JackBB (GSM)]

Hi Simon,

After further internal review, we are correcting several
responses from our previous reply. The following items will
be fixed in v5.

On [date] Jack Wu wrote:

>> [Severity: High]
>> Can this lead to use-after-free and double-free on the skb?

> No. The recv callback (mtk_port_internal_recv) does not free
> the skb on error — it returns -ENXIO and leaves skb ownership
> with the caller.

>> [Severity: High]
>> This is where the second free of the skb occurs when propagating
>> the error from the recv callback.

> As above, the recv callback does not free the skb on error.

>> [Severity: High]
>> This is where the skb is freed on the error path before returning
>> -ENXIO.

> mtk_port_internal_recv() does not call dev_kfree_skb_any() on the
> error path.

Our previous response was incorrect. In Patch 4's code,
mtk_port_internal_recv's drop_data does have
dev_kfree_skb_any(skb) — the double-free and UAF are real.

Will remove dev_kfree_skb_any(skb) from
mtk_port_internal_recv's drop_data path in v5. The caller
(mtk_port_rx_dispatch) is the sole owner and frees the skb
on error.

>> [Severity: High]
>> Could this create an infinite loop in kernel space?

> This is intentional. Channel enable/disable are control plane
> operations that must complete or timeout. The loop does not
> busy-spin: each iteration sleeps in
> wait_event_interruptible_timeout until the condition is met or
> the timeout expires.

>> [Severity: High]
>> This appears to have the same infinite loop risk.

> Same reasoning as mtk_port_ch_enable.

Will change wait_event_interruptible_timeout to
wait_event_timeout in both mtk_port_ch_enable and
mtk_port_ch_disable, and remove the -ERESTARTSYS / goto
start_wait block.

>> [Severity: High]
>> Missing bounds check on msg->port_cnt in
>> mtk_port_status_update.

> The message is already validated by head_pattern, tail_pattern,
> and version checks before the loop. The modem firmware is a
> trusted source.

Will move data_len validation before the callback call in
mtk_fsm_parse_hs2_msg, pass data_len to the callback, and add
a bounds check in mtk_port_status_update:

  if (data_len < sizeof(*msg) +
      le16_to_cpu(msg->port_cnt) * sizeof(*port_info))
      return -EPROTO;

Thanks.

Jack Wu