Re: [PATCH] net: wwan: t7xx: validate the HS2 message data length

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 (2) · Medium: 1 · Low: 0

- [High] The new validation in control_msg_handler() bounds the modem-
  supplied data_length only from above (`data_length > skb->len`).
- [Medium] control_msg_handler()
  (drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c:179-205) never verifies…

Pre-existing issues:
- [High] control_msg_handler() owns the skb dequeued by
  port_ctl_rx_thread() (drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c:262…
- [High] t7xx_cldma_gpd_rx_from_q()
  (drivers/net/wwan/t7xx/t7xx_hif_cldma.c:159-161) passes the device-…

--

net: wwan: t7xx: validate the HS2 message data length

This adds a bound on the modem-supplied data_length field of a
CTL_ID_HS2_MSG control message before it is handed to
t7xx_fsm_append_event(), rejecting the message when data_length is
larger than the remaining skb length.

> diff --git a/drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c b/drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c
> index f869e4ed9ee9..0f2ead8a7caa 100644
> --- a/drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c
> +++ b/drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c
> @@ -178,22 +178,32 @@ static int control_msg_handler(struct t7xx_port *port, struct sk_buff *skb)
>  
>  	ctrl_msg_h = (struct ctrl_msg_header *)skb->data;
>  	switch (le32_to_cpu(ctrl_msg_h->ctrl_msg_id)) {
> -	case CTL_ID_HS2_MSG:
> +	case CTL_ID_HS2_MSG: {
> +		u32 data_length;
> +
>  		skb_pull(skb, sizeof(*ctrl_msg_h));
> +		data_length = le32_to_cpu(ctrl_msg_h->data_length);

[Severity: Medium]
Is the new bound sound when the message is shorter than the control
header itself?  Nothing in control_msg_handler() checks that skb->len is
at least sizeof(struct ctrl_msg_header) (12 bytes), and the return value
of skb_pull() is discarded:

include/linux/skbuff.h:
	static inline void *skb_pull_inline(struct sk_buff *skb, unsigned int len)
	{
		return unlikely(len > skb->len) ? NULL : __skb_pull(skb, len);
	}

So for a 0..11 byte message the pull is a no-op: skb->data still points
at the control header and skb->len still counts those header bytes.  The
subsequent data_length > skb->len test is then evaluated against the
wrong length, and t7xx_fsm_append_event() stores control-header bytes as
if they were HS2 payload.

The read of ctrl_msg_h->data_length itself is at offset 8..11, which is
past skb->tail for such a short message, i.e. it reads the uninitialized
part of the skb data area allocated in t7xx_cldma_alloc_and_map_skb().

The length reaching here is fully device-controlled, with no floor
anywhere upstream:

t7xx_hif_cldma.c:t7xx_cldma_gpd_rx_from_q() {
	...
	skb_put(skb, le16_to_cpu(gpd->data_buff_len));
	...
}

t7xx_port_proxy.c:t7xx_port_proxy_recv_skb() {
	...
	skb_pull(skb, sizeof(*ccci_h));

	ret = port_conf->ops->recv_skb(port, skb);
	...
}

Would it be worth adding the same header floor the enumeration path
already has, before the pull?

	if (skb->len < sizeof(*ctrl_msg_h))
		return -EINVAL;

t7xx_port_enum_msg_handler() does this for its own header:

	if (msg_len < sizeof(*port_msg)) {
		dev_err(dev, "Port enum msg too short for header: need %zu, have %zu\n",
			sizeof(*port_msg), msg_len);
		return -EINVAL;
	}

>  
>  		if (port_conf->rx_ch == PORT_CH_CONTROL_RX ||
>  		    port_conf->rx_ch == PORT_CH_AP_CONTROL_RX) {
>  			int event = port_conf->rx_ch == PORT_CH_CONTROL_RX ?
>  				    FSM_EVENT_MD_HS2 : FSM_EVENT_AP_HS2;
>  
> -			ret = t7xx_fsm_append_event(ctl, event, skb->data,
> -						    le32_to_cpu(ctrl_msg_h->data_length));
> -			if (ret)
> -				dev_err(port->dev, "Failed to append Handshake 2 event");
> +			if (data_length > skb->len) {
> +				dev_err(port->dev, "Invalid HS2 message length %u\n",
> +					data_length);
> +				ret = -EINVAL;
> +			} else {
> +				ret = t7xx_fsm_append_event(ctl, event, skb->data,
> +							    data_length);
> +				if (ret)
> +					dev_err(port->dev, "Failed to append Handshake 2 event");
> +			}

[Severity: High]
Should data_length also be bounded from below here?  Only the upper
bound is enforced, so data_length == 0 (or anything smaller than
sizeof(struct feature_query), 72 bytes) is accepted and becomes the FSM
event payload length.

t7xx_fsm_append_event() sizes the allocation from that value:

t7xx_state_monitor.c:t7xx_fsm_append_event() {
	...
	event = kmalloc_flex(*event, data, length,
			     in_interrupt() ? GFP_ATOMIC : GFP_KERNEL);
	...
	event->length = length;

	if (data && length)
		memcpy(event->data, data, length);
	...
}

with the payload being a counted flexible array:

t7xx_state_monitor.h:
	struct t7xx_fsm_event {
		struct list_head	entry;
		enum t7xx_fsm_event_state event_id;
		unsigned int		length;
		unsigned char		data[] __counted_by(length);

The consumer of that payload does not look at event->length:

t7xx_modem_ops.c:t7xx_core_hk_handler() {
	...
	ret = t7xx_prepare_device_rt_data(core_info, dev, event->data);
	...
}

t7xx_modem_ops.c:t7xx_prepare_device_rt_data() {
	struct feature_query *md_feature = data;
	...
	if (le32_to_cpu(md_feature->head_pattern) != MD_FEATURE_QUERY_ID ||
	    le32_to_cpu(md_feature->tail_pattern) != MD_FEATURE_QUERY_ID) {
	...
}

With data_length == 0 that reads 72 bytes (head_pattern,
feature_set[64], tail_pattern) out of a 24-byte allocation, and the
out-of-bounds feature_set bytes are then echoed back to the device in
the HS3 reply via rt_feature->support_info = md_feature->feature_set[i].
The length-aware t7xx_parse_host_rt_data() called just before it simply
returns 0 for a short payload, since its loop condition is
offset < data_length with offset starting at sizeof(struct
feature_query).

Would rejecting data_length < sizeof(struct feature_query) here, or
making t7xx_prepare_device_rt_data() length-aware, be preferable to the
max-only check?

>  		}
>  
>  		dev_kfree_skb_any(skb);
>  		break;
> +	}
>  
>  	case CTL_ID_MD_EX:
>  	case CTL_ID_MD_EX_ACK:

Two more observations about surrounding code that the same malformed
control messages reach:

[Severity: High]
This isn't a bug introduced by this patch, but does control_msg_handler()
leak the RX skb for two of the switch arms it is being modified in?
port_ctl_rx_thread() hands ownership over and never frees it:

	skb = __skb_dequeue(&port->rx_skb_list);
	spin_unlock_irqrestore(&port->rx_wq.lock, flags);

	control_msg_handler(port, skb);

Only the CTL_ID_HS2_MSG and CTL_ID_MD_EX* arms call
dev_kfree_skb_any().  The enumeration arm returns without freeing:

	case CTL_ID_PORT_ENUM:
		skb_pull(skb, sizeof(*ctrl_msg_h));
		ret = t7xx_port_enum_msg_handler(ctl->md, (struct port_msg *)skb->data, skb->len);
		...
		break;

and so does the unknown-message arm:

	default:
		ret = -EINVAL;
		dev_err(port->dev, "Unknown control message ID to FSM %x\n",
			le32_to_cpu(ctrl_msg_h->ctrl_msg_id));
		break;

t7xx_port_enum_msg_handler() only parses the buffer, and
port_ctl_send_msg_to_md() allocates a fresh skb through
t7xx_ctrl_alloc_skb(), so neither takes ownership.  Since ctrl_msg_id is
modem-supplied, can a modem repeatedly sending an unrecognized
ctrl_msg_id leak one RX skb (CLDMA_SHARED_Q_BUFF_SZ, 3584 bytes, plus
skb overhead) per message without bound?

[Severity: High]
This is a pre-existing issue and not something this patch introduces,
but it is the same modem-supplied length trust boundary one layer down.
Can t7xx_cldma_gpd_rx_from_q() panic on a device-reported GPD length
larger than the RX buffer?

t7xx_hif_cldma.c:t7xx_cldma_gpd_rx_from_q() {
	...
	skb->len = 0;
	skb_reset_tail_pointer(skb);
	skb_put(skb, le16_to_cpu(gpd->data_buff_len));
	...
}

gpd->data_buff_len is a __le16 written by the device, so up to 65535,
while the skb comes from __dev_alloc_skb(queue->tr_ring->pkt_size) with
pkt_size being CLDMA_SHARED_Q_BUFF_SZ (3584),
CLDMA_DEDICATED_Q_BUFF_SZ (2048) or CLDMA_JUMBO_BUFF_SZ (64528).  The
only limit given to the hardware is advisory:

t7xx_hif_cldma.c:t7xx_cldma_ring_init() {
	...
	gpd->rx_data_allow_len = cpu_to_le16(ring->pkt_size);
	...
}

Nothing re-checks the returned length against pkt_size or the skb
tailroom, so a length beyond the buffer would take skb->tail past
skb->end and hit skb_over_panic() -> BUG().  Would a check of
data_buff_len against tr_ring->pkt_size before the skb_put() be
appropriate?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909054401.718959-1-zhugl3%40xiaopeng.com