Re: [PATCH v5] net: wwan: t7xx: validate port_count against message length in t7xx_port_enum_msg_handler

Simon Horman
2026-04-27T19:03:50Z

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net: wwan: t7xx: validate port_count against message length in t7xx_port_enum_msg_handler

The patch attempts to fix a slab-out-of-bounds read in
t7xx_port_enum_msg_handler() by checking msg_len against struct_size().
It passes msg_len from t7xx_parse_host_rt_data() and control_msg_handler().

Cc: stable@vger.kernel.org

> diff --git a/drivers/net/wwan/t7xx/t7xx_modem_ops.c b/drivers/net/wwan/t7xx/t7xx_modem_ops.c
> index 7968e208dd37c..dc90691ef3f54 100644
> --- a/drivers/net/wwan/t7xx/t7xx_modem_ops.c
> +++ b/drivers/net/wwan/t7xx/t7xx_modem_ops.c
> @@ -453,12 +453,14 @@ static int t7xx_parse_host_rt_data(struct t7xx_fsm_ctl *ctl, struct t7xx_sys_inf
> {
> enum mtk_feature_support_type ft_spt_st, ft_spt_cfg;
> struct mtk_runtime_feature *rt_feature;
> + size_t feat_data_len;
> int i, offset;
>
> offset = sizeof(struct feature_query);
> for (i = 0; i < FEATURE_COUNT && offset < data_length; i++) {
> rt_feature = data + offset;

The loop condition "offset < data_length" only ensures there is at least 1 byte
remaining in the buffer. Could accessing rt_feature->data_len read out-of-bounds
if data_length - offset is less than sizeof(*rt_feature)?

> - offset += sizeof(*rt_feature) + le32_to_cpu(rt_feature->data_len);
> + feat_data_len = le32_to_cpu(rt_feature->data_len);
> + offset += sizeof(*rt_feature) + feat_data_len;

Could adding the 32-bit untrusted feat_data_len to offset cause an integer
overflow since offset is a signed int? If a malicious modem provides a large
value (e.g., 0xFFFFFFF0), offset could become negative, which would keep the
loop condition true and lead to arbitrary backward out-of-bounds reads on the
next iteration.

> ft_spt_cfg = FIELD_GET(FEATURE_MSK, core->feature_set[i]);
> if (ft_spt_cfg != MTK_FEATURE_MUST_BE_SUPPORTED)
> @@ -468,8 +470,10 @@ static int t7xx_parse_host_rt_data(struct t7xx_fsm_ctl *ctl, struct t7xx_sys_inf
> if (ft_spt_st != MTK_FEATURE_MUST_BE_SUPPORTED)
> return -EINVAL;
>
> - if (i == RT_ID_MD_PORT_ENUM || i == RT_ID_AP_PORT_ENUM)
> - t7xx_port_enum_msg_handler(ctl->md, rt_feature->data);
> + if (i == RT_ID_MD_PORT_ENUM || i == RT_ID_AP_PORT_ENUM) {
> + t7xx_port_enum_msg_handler(ctl->md, rt_feature->data,
> + feat_data_len);
> + }

Here, feat_data_len is passed as msg_len, which is extracted directly from the
untrusted modem payload (rt_feature->data_len). Is this length validated against
the actual remaining buffer size (data_length - offset - sizeof(*rt_feature))?

If a malicious modem provides a small physical buffer but a very large data_len
field, the new msg_len bounds check in t7xx_port_enum_msg_handler() could still
pass, defeating the purpose of this patch and allowing the out-of-bounds read to
occur.

> }
>
> return 0;
> diff --git a/drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c b/drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c
> index ae632ef966983..fa24284448e55 100644
> --- a/drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c
> +++ b/drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c
[ ... ]
> @@ -141,6 +142,13 @@ int t7xx_port_enum_msg_handler(struct t7xx_modem *md, void *msg)
> }
>
> port_count = FIELD_GET(PORT_MSG_PRT_CNT, le32_to_cpu(port_msg->info));

This function reads port_msg->info, port_msg->head_pattern, and
port_msg->tail_pattern before validating msg_len. If the provided msg_len is
smaller than sizeof(struct port_msg) (12 bytes), could these accesses read
out-of-bounds memory?

> +
> + if (msg_len < struct_size(port_msg, data, port_count)) {
> + dev_err(dev, "Port enum msg too short: need %zu, have %zu\n",
> + struct_size(port_msg, data, port_count), msg_len);
> + return -EINVAL;
> + }
> +
[ ... ]
> @@ -191,7 +199,7 @@ static int control_msg_handler(struct t7xx_port *port, struct sk_buff *skb)
>
> case CTL_ID_PORT_ENUM:
> skb_pull(skb, sizeof(*ctrl_msg_h));

Is the SKB length validated before calling skb_pull()? If skb->len is less than
sizeof(*ctrl_msg_h), skb_pull() fails and leaves the SKB unmodified. Passing the
unpulled skb->data could cause the handler to misinterpret the control header as
the port message payload.

> - ret = t7xx_port_enum_msg_handler(ctl->md, (struct port_msg *)skb->data);
> + ret = t7xx_port_enum_msg_handler(ctl->md, (struct port_msg *)skb->data, skb->len);
> if (!ret)
> ret = port_ctl_send_msg_to_md(port, CTL_ID_PORT_ENUM, 0);
> else

Does this function leak the skb buffer in the CTL_ID_PORT_ENUM and default
switch cases?

In control_msg_handler(), the skb is explicitly freed using dev_kfree_skb_any()
for CTL_ID_HS2_MSG and CTL_ID_MD_EX*:

drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c:control_msg_handler() {
...
switch (le32_to_cpu(ctrl_msg_h->ctrl_msg_id)) {
case CTL_ID_HS2_MSG:
...
dev_kfree_skb_any(skb);
break;

case CTL_ID_MD_EX:
case CTL_ID_MD_EX_ACK:
case CTL_ID_MD_EX_PASS:
case CTL_ID_DRV_VER_ERROR:
ret = fsm_ee_message_handler(port, ctl, skb);
dev_kfree_skb_any(skb);
break;
...
}

However, for CTL_ID_PORT_ENUM and default, the function returns without freeing
the skb, and the caller does not free it either. Could a malicious modem trigger
memory exhaustion by repeatedly sending CTL_ID_PORT_ENUM or invalid control
messages?