Re: [PATCH 1/2] soc: qcom: qmi: Fix "invalid data length" in encoder

[Alexander Wilhelm <alexander.wilhelm@westermo.com>]

On Sat, Feb 14, 2026 at 03:16:55PM -0600, Bjorn Andersson wrote:
> When encoding QMI messages, the "source buffer" is a C-struct in the
> host memory, so while the data that goes into the outgoing buffer should
> be converted to little endian, the length should not be.
> 
> Commit 'fe099c387e06 ("soc: qcom: preserve CPU endianness for
> QMI_DATA_LEN")' fixed this, but did it by copying a whole word from the
> source into a local u32 and then operated on that.
> 
> If the length in the DATA_LEN refers to either a char or short array,
> it's reasonable to expect that the struct is packed such that this word
> will contain not only the length-byte (or length-short), but also the
> beginning of the payload.
> 
> As the encoder loops around to encode the payload it runs into an
> unreasonable value of "data_len_value" and bails, with the error message
> "qmi_encode: Invalid data length".
> 
> Rather then complicating the logic with local variables of different
> types we can instead pick the u8 or u16 "data_len_value" directly from
> "buf_src". As "buf_src" refers to a typical C-structure in the client
> drivers, we expect this field to be naturally aligned.
> 
> We can then return to the original expression of qmi_encode_basic_elem()
> encoding directly from "src_buf" to "dst_buf", with the endianness
> conversion, based on the size of the type.
> 
> Reported-by: David Heidelberg <david@ixit.cz>
> Closes: https://urldefense.com/v3/__https://lore.kernel.org/all/dfb72933-938f-43f2-87f3-2e3ab9697125@ixit.cz/__;!!I9LPvj3b!BCfk4-YtwbkEy3mc_UUojT1xCH5BW5COilqBek1tBnJyWzp2eK716Cj0C_35FQwo8__BS8qk_PK5oJs9i719BCjcA-rnMg3YY71aTHHs$ 
> Fixes: fe099c387e06 ("soc: qcom: preserve CPU endianness for QMI_DATA_LEN")
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> ---
>  drivers/soc/qcom/qmi_encdec.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/soc/qcom/qmi_encdec.c b/drivers/soc/qcom/qmi_encdec.c
> index 28ce6f130b6ac355820bb295c8c96f9c6a6e385f..45bb26d010da77ab8d481897026b718c2290bad7 100644
> --- a/drivers/soc/qcom/qmi_encdec.c
> +++ b/drivers/soc/qcom/qmi_encdec.c
> @@ -368,8 +368,6 @@ static int qmi_encode(const struct qmi_elem_info *ei_array, void *out_buf,
>  	const void *buf_src;
>  	int encode_tlv = 0;
>  	int rc;
> -	u8 val8;
> -	u16 val16;
>  
>  	if (!ei_array)
>  		return 0;
> @@ -406,7 +404,6 @@ static int qmi_encode(const struct qmi_elem_info *ei_array, void *out_buf,
>  			break;
>  
>  		case QMI_DATA_LEN:
> -			memcpy(&data_len_value, buf_src, sizeof(u32));

Hi Bjorn,

unfortunatelly, this change breaks the `ath11k`, and most likely `ath12k`,
execution on big-endian platforms:

    ath11k_pci 0001:01:00.0: BAR 0: assigned [mem 0xc00000000-0xc001fffff 64bit]
    ath11k_pci 0001:01:00.0: MSI vectors: 1
    ath11k_pci 0001:01:00.0: qcn9074 hw1.0
    ath11k_pci 0001:01:00.0: FW memory mode: 0
    ath11k_pci 0002:01:00.0: BAR 0: assigned [mem 0xc10000000-0xc101fffff 64bit]
    ath11k_pci 0002:01:00.0: MSI vectors: 1
    ath11k_pci 0002:01:00.0: qcn9074 hw1.0
    ath11k_pci 0002:01:00.0: FW memory mode: 0
    ath11k_pci 0001:01:00.0: invalid memory segment length: 83886080
    ath11k_pci 0001:01:00.0: invalid memory segment length: 419430400
    ath11k_pci 0001:01:00.0: qmi respond memory request failed: 1 0
    ath11k_pci 0001:01:00.0: qmi failed to respond fw mem req: -22
    ath11k_pci 0001:01:00.0: qmi respond memory request failed: 1 48
    ath11k_pci 0001:01:00.0: qmi failed to respond fw mem req: -22
    ath11k_pci 0002:01:00.0: invalid memory segment length: 83886080
    ath11k_pci 0002:01:00.0: invalid memory segment length: 419430400
    ath11k_pci 0002:01:00.0: qmi respond memory request failed: 1 0
    ath11k_pci 0002:01:00.0: qmi failed to respond fw mem req: -22 

I tried to analyze the regression I introduced and I think I now understand
what went wrong. Previously, the code looked like the this:

    memcpy(&data_len_value, buf_src, temp_ei->elem_size);

However, this never worked correctly on big‑endian systems. `buf_src` is a
`void *`, but `ath11k` and `ath12k` always store the data as `u32`. Assume
the element value is `0xABCD` with an elem_size of 2, that is, the
`sizeof(u16)`. The memory layout on the driver side then looks like this (X
marks unused bytes):

    +---------------+----+----+----+----+
    | Little Endian | XX | XX | AB | CD |
    +---------------+----+----+----+----+
    | Big Endian    | CD | AB | XX | XX |
    +---------------+----+----+----+----+

When `buf_src` is treated as an array of `u32` and then “reinterpreted” as
an array of `u8`, only the first 2 bytes of the `u32` are copied, which, on
big‑endian, no longer contain the actual data. After the copy,
`data_len_value` contains the following data:

    +---------------+----+----+----+----+
    | Little Endian | XX | XX | AB | CD |
    +---------------+----+----+----+----+
    | Big Endian    | XX | XX | XX | XX |
    +---------------+----+----+----+----+

So the original value `0xABCD` never gets copied at all on big‑endian
systems. This is why a simple pointer cast cannot work reliably on
big‑endian architectures. I did the following change:

    memcpy(&data_len_value, buf_src, sizeof(u32));

My attempt was to always copy the full `u32` value , but it seems that the
modem on the "Pixel 3" does not actually use a `u32` there, but rather an
array or a packed structure. I’ve CC’ed Jeff and the `ath11k/ath12k`
mailing list as well. Hopefully we can find a solution that works across
both endianness architectures.

>  			data_len_sz = temp_ei->elem_size == sizeof(u8) ?
>  					sizeof(u8) : sizeof(u16);
>  			/* Check to avoid out of range buffer access */
> @@ -416,19 +413,16 @@ static int qmi_encode(const struct qmi_elem_info *ei_array, void *out_buf,
>  				       __func__);
>  				return -ETOOSMALL;
>  			}
> -			if (data_len_sz == sizeof(u8)) {
> -				val8 = data_len_value;
> -				rc = qmi_encode_basic_elem(buf_dst, &val8,
> -							   1, data_len_sz);
> -				if (rc < 0)
> -					return rc;
> -			} else {
> -				val16 = data_len_value;
> -				rc = qmi_encode_basic_elem(buf_dst, &val16,
> -							   1, data_len_sz);
> -				if (rc < 0)
> -					return rc;
> -			}
> +
> +			if (data_len_sz == sizeof(u8))
> +				data_len_value = *(u8 *)buf_src;
> +			else
> +				data_len_value = *(u16 *)buf_src;
> +
> +			rc = qmi_encode_basic_elem(buf_dst, buf_src, 1, data_len_sz);

Here is the problem again: `buf_src` is once more being cast either to a
`u8 *` or a `u16 *`. This does not cause issues on little‑endian systems,
but it corrupts the data on big‑endian platforms.


Best regards
Alexander Wilhelm