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

[Simon Horman <horms@kernel.org>]

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?