* Re: [PATCH] net: wwan: t7xx: validate the HS2 message data length
2026-09-09 5:44 [PATCH] net: wwan: t7xx: validate the HS2 message data length Guanglei Zhu
@ 2026-09-09 13:10 ` Loic Poulain
2026-09-10 5:46 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: Loic Poulain @ 2026-09-09 13:10 UTC (permalink / raw)
To: Guanglei Zhu
Cc: Chandrashekar Devegowda, Sergey Ryazanov, Liu Haijun,
Ricardo Martinez, Johannes Berg, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel,
stable
On Wed, Sep 9, 2026 at 7:44 AM Guanglei Zhu <zhugl3@xiaopeng.com> wrote:
>
> control_msg_handler() passes the modem-supplied data_length field of a
> CTL_ID_HS2_MSG message straight to t7xx_fsm_append_event(), which
> memcpy()s that many bytes out of the skb. Nothing compares the field
> with the actual length of the message, so a modem reporting a larger
> data_length makes the driver read past the end of the skb and store
> kernel heap memory in the FSM event.
>
> 0e7c074cfcd9 ("net: wwan: t7xx: validate port_count against message
> length in t7xx_port_enum_msg_handler") added this kind of check for
> the port enumeration path but missed the handshake path.
>
> Reject the message when data_length does not fit into the skb.
>
> Fixes: da45d2566a1d ("net: wwan: t7xx: Add control port")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>
Reviewed-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
> ---
>
> Verified in a QEMU guest with a fault injector feeding the driver's
> control port thread a handshake message whose data_length exceeds the
> skb: the unpatched driver trips KASAN on a read of 8192 bytes past a
> 500-byte payload, and the extra bytes land in the FSM event. With
> this check the message is rejected, and well-formed handshakes are
> unaffected.
> drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c b/drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c
> index f869e4ed9..0f2ead8a7 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);
>
> 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");
> + }
> }
>
> dev_kfree_skb_any(skb);
> break;
> + }
>
> case CTL_ID_MD_EX:
> case CTL_ID_MD_EX_ACK:
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] net: wwan: t7xx: validate the HS2 message data length
2026-09-09 5:44 [PATCH] net: wwan: t7xx: validate the HS2 message data length Guanglei Zhu
2026-09-09 13:10 ` Loic Poulain
@ 2026-09-10 5:46 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sashiko @ 2026-09-10 5:46 UTC (permalink / raw)
To: zhugl3
Cc: chandrashekar.devegowda, loic.poulain, ryazanov.s.a, haijun.liu,
ricardo.martinez, johannes, andrew+netdev, davem, edumazet, kuba,
pabeni, netdev, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 3+ messages in thread