diff mbox series

wifi: ath12k: fix possible out-of-bound read in ath12k_htt_pull_ppdu_stats()

Message ID 20230901015602.45112-1-quic_bqiang@quicinc.com (mailing list archive)
State Accepted
Commit 1bc44a505a229bb1dd4957e11aa594edeea3690e
Delegated to: Kalle Valo
Headers show
Series wifi: ath12k: fix possible out-of-bound read in ath12k_htt_pull_ppdu_stats() | expand

Commit Message

Baochen Qiang Sept. 1, 2023, 1:56 a.m. UTC 
len is extracted from HTT message and could be an unexpected value in
case errors happen, so add validation before using to avoid possible
out-of-bound read in the following message iteration and parsing.

The same issue also applies to ppdu_info->ppdu_stats.common.num_users,
so validate it before using too.

These are found during code review.

Compile test only.

Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/dp_rx.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)


base-commit: a62b0aeb556839fb6abb9835874443b08fe95598

Comments

Jeff Johnson Sept. 1, 2023, 3:40 a.m. UTC | #1 
On 8/31/2023 6:56 PM, Baochen Qiang wrote:
> len is extracted from HTT message and could be an unexpected value in
> case errors happen, so add validation before using to avoid possible
> out-of-bound read in the following message iteration and parsing.
> 
> The same issue also applies to ppdu_info->ppdu_stats.common.num_users,
> so validate it before using too.
> 
> These are found during code review.
> 
> Compile test only.
> 
> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>

Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>

> ---
>   drivers/net/wireless/ath/ath12k/dp_rx.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/dp_rx.c b/drivers/net/wireless/ath/ath12k/dp_rx.c
> index e6e64d437c47..5189a0690d44 100644
> --- a/drivers/net/wireless/ath/ath12k/dp_rx.c
> +++ b/drivers/net/wireless/ath/ath12k/dp_rx.c
> @@ -1555,6 +1555,13 @@ static int ath12k_htt_pull_ppdu_stats(struct ath12k_base *ab,
>   
>   	msg = (struct ath12k_htt_ppdu_stats_msg *)skb->data;
>   	len = le32_get_bits(msg->info, HTT_T2H_PPDU_STATS_INFO_PAYLOAD_SIZE);
> +	if (len > (skb->len - struct_size(msg, data, 0))) {
> +		ath12k_warn(ab,
> +			    "HTT PPDU STATS event has unexpected payload size %u, should be smaller than %u\n",
> +			    len, skb->len);
> +		return -EINVAL;
> +	}
> +
>   	pdev_id = le32_get_bits(msg->info, HTT_T2H_PPDU_STATS_INFO_PDEV_ID);
>   	ppdu_id = le32_to_cpu(msg->ppdu_id);
>   
> @@ -1583,6 +1590,15 @@ static int ath12k_htt_pull_ppdu_stats(struct ath12k_base *ab,
>   		goto exit;
>   	}
>   
> +	if (ppdu_info->ppdu_stats.common.num_users >= HTT_PPDU_STATS_MAX_USERS) {
> +		spin_unlock_bh(&ar->data_lock);
> +		ath12k_warn(ab,
> +			    "HTT PPDU STATS event has unexpected num_users %u, should be smaller than %u\n",
> +			    ppdu_info->ppdu_stats.common.num_users, HTT_PPDU_STATS_MAX_USERS);
> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +
>   	/* back up data rate tlv for all peers */
>   	if (ppdu_info->frame_type == HTT_STATS_PPDU_FTYPE_DATA &&
>   	    (ppdu_info->tlv_bitmap & (1 << HTT_PPDU_STATS_TAG_USR_COMMON)) &&
> 
> base-commit: a62b0aeb556839fb6abb9835874443b08fe95598
Kalle Valo Sept. 20, 2023, 1:38 p.m. UTC | #2 
Baochen Qiang <quic_bqiang@quicinc.com> wrote:

> len is extracted from HTT message and could be an unexpected value in
> case errors happen, so add validation before using to avoid possible
> out-of-bound read in the following message iteration and parsing.
> 
> The same issue also applies to ppdu_info->ppdu_stats.common.num_users,
> so validate it before using too.
> 
> These are found during code review.
> 
> Compile test only.
> 
> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
> Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

Patch applied to ath-next branch of ath.git, thanks.

1bc44a505a22 wifi: ath12k: fix possible out-of-bound read in ath12k_htt_pull_ppdu_stats()
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath12k/dp_rx.c b/drivers/net/wireless/ath/ath12k/dp_rx.c
index e6e64d437c47..5189a0690d44 100644
--- a/drivers/net/wireless/ath/ath12k/dp_rx.c
+++ b/drivers/net/wireless/ath/ath12k/dp_rx.c
@@ -1555,6 +1555,13 @@  static int ath12k_htt_pull_ppdu_stats(struct ath12k_base *ab,
 
 	msg = (struct ath12k_htt_ppdu_stats_msg *)skb->data;
 	len = le32_get_bits(msg->info, HTT_T2H_PPDU_STATS_INFO_PAYLOAD_SIZE);
+	if (len > (skb->len - struct_size(msg, data, 0))) {
+		ath12k_warn(ab,
+			    "HTT PPDU STATS event has unexpected payload size %u, should be smaller than %u\n",
+			    len, skb->len);
+		return -EINVAL;
+	}
+
 	pdev_id = le32_get_bits(msg->info, HTT_T2H_PPDU_STATS_INFO_PDEV_ID);
 	ppdu_id = le32_to_cpu(msg->ppdu_id);
 
@@ -1583,6 +1590,15 @@  static int ath12k_htt_pull_ppdu_stats(struct ath12k_base *ab,
 		goto exit;
 	}
 
+	if (ppdu_info->ppdu_stats.common.num_users >= HTT_PPDU_STATS_MAX_USERS) {
+		spin_unlock_bh(&ar->data_lock);
+		ath12k_warn(ab,
+			    "HTT PPDU STATS event has unexpected num_users %u, should be smaller than %u\n",
+			    ppdu_info->ppdu_stats.common.num_users, HTT_PPDU_STATS_MAX_USERS);
+		ret = -EINVAL;
+		goto exit;
+	}
+
 	/* back up data rate tlv for all peers */
 	if (ppdu_info->frame_type == HTT_STATS_PPDU_FTYPE_DATA &&
 	    (ppdu_info->tlv_bitmap & (1 << HTT_PPDU_STATS_TAG_USR_COMMON)) &&