Re: [PATCH] net: wwan: t7xx: validate control-message data_length against the skb

[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
---
--- Patch [1]: [PATCH] net: wwan: t7xx: validate control-message data_length against the skb ---
commit d01ff7bcea9777a110312cfd4a96824ff782a70a
Author: Doruk Tan Ozturk <doruk@0sec.ai>

net: wwan: t7xx: validate control-message data_length against the skb
    
control_msg_handler() handles a CTL_ID_HS2_MSG control message by pulling
the ctrl_msg_header and passing the modem-supplied data_length as the
length of the handshake-2 payload to t7xx_fsm_append_event(). This commit
rejects a data_length that exceeds the received payload.

> diff --git a/drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c b/drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c
> index f869e4ed9ee9a..871ed63d3c4d2 100644
> --- a/drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c
> +++ b/drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c
> @@ -186,10 +186,15 @@ static int control_msg_handler(struct t7xx_port *port, struct sk_buff *skb)
>  			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 (le32_to_cpu(ctrl_msg_h->data_length) > skb->len) {
> +				dev_err(port->dev, "Invalid Handshake 2 data length\n");
> +				ret = -EINVAL;
> +			} else {
> +				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");
> +			}
>  		}
>  
>  		dev_kfree_skb_any(skb);

[Severity: High]
This is a pre-existing issue, but I noticed that earlier in this function
we call skb_pull() without validating the header size against skb->len:

drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c:control_msg_handler() {
    ...
	ctrl_msg_h = (struct ctrl_msg_header *)skb->data;
	switch (le32_to_cpu(ctrl_msg_h->ctrl_msg_id)) {
	case CTL_ID_HS2_MSG:
		skb_pull(skb, sizeof(*ctrl_msg_h));
    ...
}

Because skb_pull() fails silently if the length is insufficient, could this
lead to out-of-bounds reads and processing of uninitialized memory if the
modem sends a packet smaller than sizeof(struct ctrl_msg_header)?

I found a similar pattern in t7xx_port_proxy_recv_skb() where we read
ccci_header and call skb_pull() without checking if skb->len is large enough:

drivers/net/wwan/t7xx/t7xx_port_proxy.c:t7xx_port_proxy_recv_skb() {
    ...
	struct ccci_header *ccci_h = (struct ccci_header *)skb->data;
	...
	channel = FIELD_GET(CCCI_H_CHN_FLD, le32_to_cpu(ccci_h->status));
	...
	skb_pull(skb, sizeof(*ccci_h));
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but I found an issue in the
HIF CLDMA receive path where an unchecked modem-provided length is passed
directly to skb_put():

drivers/net/wwan/t7xx/t7xx_hif_cldma.c:t7xx_cldma_gpd_rx_from_q() {
    ...
	skb_put(skb, le16_to_cpu(gpd->data_buff_len));
    ...
}

If the modem provides a gpd->data_buff_len larger than the dynamically
allocated SKB's tailroom, wouldn't this trigger an skb_over_panic() and
cause a system denial of service?

[Severity: High]
This is a pre-existing issue, but it looks like we might be missing a read
memory barrier in the same receive path:

drivers/net/wwan/t7xx/t7xx_hif_cldma.c:t7xx_cldma_gpd_rx_from_q() {
    ...
	if ((gpd->flags & GPD_FLAGS_HWO) || !req->skb) {
		...
	}
	...
	skb_put(skb, le16_to_cpu(gpd->data_buff_len));
    ...
}

Since we poll for the hardware ownership flag (GPD_FLAGS_HWO) and then
read the data_buff_len without an intervening dma_rmb(), could the CPU
speculatively reorder the read of the descriptor's payload fields to occur
before the ownership flag check is globally visible?

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a missing write
memory barrier in the transmission path when setting the ownership flag:

drivers/net/wwan/t7xx/t7xx_hif_cldma.c:t7xx_cldma_gpd_handle_tx_request() {
    ...
	gpd->data_buff_len = cpu_to_le16(skb->len);

	spin_lock_irqsave(&md_ctrl->cldma_lock, flags);
	if (md_ctrl->txq_active & BIT(queue->index))
		gpd->flags |= GPD_FLAGS_HWO;
    ...
}

Because spin_lock_irqsave() only provides ACQUIRE semantics, could the
memory writes for data_buff_len be delayed past the lock and become
visible to the hardware after the GPD_FLAGS_HWO flag is set?