Re: [PATCH] soc: qcom: apr: Reject packets with header size larger than packet size

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] soc: qcom: apr: uninitialized resp.payload when pkt_size equals hdr_size
--

commit 2669ea074a5a71b19b685f8644fc4fa968102cc7
Author: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

soc: qcom: apr: Reject packets with header size larger than packet size

This commit adds checks in apr_do_rx_callback() and gpr_do_rx_callback() to
reject incoming APR and GPR packets whose header size exceeds the packet size,
preventing a negative resp.payload_size and an uninitialized resp.payload
stack pointer.

> diff --git a/drivers/soc/qcom/apr.c b/drivers/soc/qcom/apr.c
> index 2d5645ff99197..6f29b57aaecce 100644
> --- a/drivers/soc/qcom/apr.c
> +++ b/drivers/soc/qcom/apr.c
> @@ -210,7 +210,8 @@ static int apr_do_rx_callback(struct packet_router *apr, struct apr_rx_buf *abuf
>  		return -EINVAL;
>  	}
>  
> -	if (hdr->pkt_size < APR_HDR_SIZE || hdr->pkt_size != len) {
> +	if (hdr->pkt_size < APR_HDR_SIZE || hdr->pkt_size != len ||
> +	    hdr->pkt_size < hdr_size) {
>  		dev_err(apr->dev, "APR: Wrong packet size\n");
>  		return -EINVAL;
>  	}
> @@ -280,7 +281,8 @@ static int gpr_do_rx_callback(struct packet_router *gpr, struct apr_rx_buf *abuf
>  		return -EINVAL;
>  	}
>  
> -	if (hdr->pkt_size < GPR_PKT_HEADER_BYTE_SIZE || hdr->pkt_size != len) {
> +	if (hdr->pkt_size < GPR_PKT_HEADER_BYTE_SIZE || hdr->pkt_size != len ||
> +	    hdr->pkt_size < hdr_size * 4) {
>  		dev_err(gpr->dev, "GPR: Wrong packet size\n");
>  		return -EINVAL;
>  	}

[Severity: High]
What happens when a packet with optional header words has hdr->pkt_size equal
to hdr_size (in apr_do_rx_callback()) or hdr_size * 4 (in
gpr_do_rx_callback())?

In apr_callback(), packets with len > APR_HDR_SIZE are accepted, so a packet
with optional header words and no payload (for example, len == 24 and a
24-byte header) passes both apr_callback() and these new < checks.

In both apr_do_rx_callback() and gpr_do_rx_callback(), resp is declared on the
stack without initialization, and resp.payload_size evaluates to 0:

	resp.hdr = *hdr;
	resp.payload_size = hdr->pkt_size - hdr_size;

	if (resp.payload_size > 0)
		resp.payload = buf + hdr_size;

	adrv->callback(adev, &resp);

Because resp.payload is only assigned when resp.payload_size > 0, resp.payload
is still left uninitialized on the stack and passed to the service callbacks
(such as q6core_callback(), q6asm_srvc_callback(), graph_callback(), and
apm_callback()), which dereference data->payload without checking
data->payload_size.

Should these checks use <= (hdr->pkt_size <= hdr_size and
hdr->pkt_size <= hdr_size * 4, matching len <= APR_HDR_SIZE in
apr_callback()), or should resp.payload be initialized to NULL if zero-length
payloads are valid?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101175407.110780-2-krzysztof.kozlowski@oss.qualcomm.com?part=1