Re: [PATCH RFC 3/4] arm64: dts: qcom: sdm845-google: Add modemsmem

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unhandled -EEXIST error causes module reload / probe failure if the SMEM item is already allocated.
- [High] Out-of-bounds read of SMEM memory due to missing item size verification.
- [High] Uninitialized SMEM memory leak to modem firmware.
--

commit 61111124cf2e78b88f10eebd1940456309ceceee
Author: David Heidelberg <david@ixit.cz>
arm64: dts: qcom: sdm845-google: Add modemsmem

This commit adds the modemsmem client to the device tree for the Pixel 3
and 3 XL devices. This enables the modem to boot properly.

> diff --git a/arch/arm64/boot/dts/qcom/sdm845-google-common.dtsi b/arch/arm64/boot/dts/qcom/sdm845-google-common.dtsi
> index 9e952f9862f24..7e8f1a34a79f8 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-google-common.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845-google-common.dtsi
[ ... ]
> @@ -50,6 +50,11 @@ framebuffer0: framebuffer-0 {
>  		};
>  	};
>  
> +	modemsmem {
> +		compatible = "google,modemsmem";
> +		qcom,smem-id = <134>;
> +	};
> +
>  	reserved-memory {
>  		cont_splash_mem: splash@9d400000 {
>  			reg = <0 0x9d400000 0 0x02400000>;

While reviewing this enablement patch, I noticed a few issues in the
modemsmem driver introduced earlier in this series:

drivers/soc/qcom/modemsmem.c:modemsmem_probe() {
    ...
	ret = qcom_smem_alloc(QCOM_SMEM_HOST_ANY, smem_id, sizeof(*info));
	if (ret)
		return dev_err_probe(dev, ret, "Could not allocate modem smem\n");
    ...
}

[Severity: High]
This problem wasn't introduced by this patch, but by an earlier patch in this
series. What happens if the SMEM item is already allocated by the bootloader
or due to a driver module reload? Does this cause modemsmem_probe() to fail
permanently because qcom_smem_alloc() returns -EEXIST? Other drivers seem to
handle this correctly by checking if (ret < 0 && ret != -EEXIST).

drivers/soc/qcom/modemsmem.c:write_socinfo() {
    ...
	socinfo = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, NULL);
    ...
}

[Severity: High]
This problem wasn't introduced by this patch, but by an earlier patch in this
series. Does passing NULL for the size argument discard the validation
information needed for safe parsing? If a bootloader creates the
SMEM_HW_SW_BUILD_ID item with an allocated size smaller than
offsetofend(struct socinfo, hw_plat_subtype), could this cause an
out-of-bounds read that leaks data from adjacent SMEM items to the modem
firmware?

drivers/soc/qcom/modemsmem.c:write_socinfo() {
    ...
	target->version = cpu_to_le32(MODEM_SMEM_VERSION);
	target->major_id = cpu_to_le32(PLAT_VER_TO_MAJOR_ID(plat_ver));
	target->minor_id = cpu_to_le32(PLAT_VER_TO_MINOR_ID(plat_ver));
	target->platform = socinfo->hw_plat;
	target->subtype = socinfo->hw_plat_subtype;
    ...
}

[Severity: High]
This problem wasn't introduced by this patch, but by an earlier patch in this
series. Since qcom_smem_alloc() does not zero out memory, are the remaining
fields of struct modem_smem_info (such as modem_flag, efs_magic, and
ftm_magic) left containing uninitialized SMEM memory? Could this expose
uninitialized junk data to the modem firmware and trigger unintended hardware
states or data leaks? Should this memory be zeroed explicitly, for instance
via memset(info, 0, sizeof(*info)), before passing it to hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906-modemsmem-v1-0-a3634b553841@ixit.cz?part=3