Re: [PATCH RFC 2/4] soc: qcom: Add modemsmem for Google phones

Re: [PATCH RFC 2/4] soc: qcom: Add modemsmem for Google phones

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Probe fails unconditionally if the SMEM item already exists, breaking module reload and pre-allocated items.
- [High] Newly allocated SMEM memory is not zeroed, exposing uninitialized data to the modem firmware.
--

commit 7a67ae0180630b09d3437d4fa514d468a7075572
Author: Richard Acayan <mailingradian@gmail.com>

soc: qcom: Add modemsmem for Google phones

Modems on Google Pixel devices require extra information about the
SoC to be passed through a vendor-controlled Shared Memory (SMEM)
channel. This driver provides this information.

> diff --git a/drivers/soc/qcom/modemsmem.c b/drivers/soc/qcom/modemsmem.c
> index 0000000000000..7a733422ff7c1
> --- /dev/null
> +++ b/drivers/soc/qcom/modemsmem.c
[ ... ]
> +static int write_socinfo(struct modem_smem_info *target)
> +{
> +	struct socinfo *socinfo;
> +	u32 plat_ver;
> +
> +	socinfo = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, NULL);
> +	if (IS_ERR(socinfo))
> +		return PTR_ERR(socinfo);
> +
> +	/* hw_plat_subtype was added in socinfo format 0.6 */
> +	if (le32_to_cpu(socinfo->fmt) < SOCINFO_VERSION(0, 6))
> +		return -EOPNOTSUPP;
> +
> +	plat_ver = le32_to_cpu(socinfo->plat_ver);
> +
> +	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]
Does this code leave uninitialized fields in struct modem_smem_info exposed to
the modem firmware?

When the SMEM item is freshly allocated by qcom_smem_alloc(), the memory is
not zeroed. This function assigns 5 of the 8 fields in struct modem_smem_info,
leaving modem_flag, efs_magic, and ftm_magic uninitialized.

Could this SRAM garbage cause undefined modem behavior or misconfiguration?
Follow-up patches in the series do not fix this regression.

> +
> +	return 0;
> +}
> +
> +static int modemsmem_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct modem_smem_info *info;
> +	u32 smem_id;
> +	int ret;
> +
> +	ret = of_property_read_u32(dev->of_node, "qcom,smem-id", &smem_id);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Could not read smem id\n");
> +
> +	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]
Will this logic fail the probe process if the SMEM item already exists?

qcom_smem_alloc() returns -EEXIST when the item has already been allocated by
an earlier boot stage or during a previous module load.

Treating any non-zero return as a fatal error prevents the driver from updating
a pre-allocated SMEM region. Follow-up patches in the series do not fix this
regression.

> +
> +	info = qcom_smem_get(QCOM_SMEM_HOST_ANY, smem_id, NULL);
> +	if (IS_ERR(info))
> +		return dev_err_probe(dev, PTR_ERR(info), "Could not get modem smem\n");

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

Re: [PATCH RFC 2/4] soc: qcom: Add modemsmem for Google phones

[Konrad Dybcio]

On 9/6/26 2:41 AM, David Heidelberg via B4 Relay wrote:
> From: Richard Acayan <mailingradian@gmail.com>
> 
> Modems on Google Pixel devices require extra information about the
> SoC to be passed through a vendor-controlled Shared Memory (SMEM)
> channel. This driver provides this information.

[...]

> +	ret = of_property_read_u32(dev->of_node, "qcom,smem-id", &smem_id);

Is it ever not 134?

FWIW you may find this useful:

134 - SMEM_ID_VENDOR0
135 - SMEM_ID_VENDOR1
136 - SMEM_ID_VENDOR2

which you may find in various downstream kernel used for various
purposes

Konrad

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing probe dependency between the `modemsmem` platform device and the modem remoteproc driver.
- [High] Probe fails if the SMEM item is already allocated by the bootloader (-EEXIST).
- [High] Uninitialized SMEM memory is exposed to the modem because several fields are not initialized.
--

commit f684325f1bc358462d3e26fd3838d75a70d04ce2
Author: Richard Acayan <mailingradian@gmail.com>

arm64: dts: qcom: sdm670-google: Add modemsmem

The Pixel 3a modem needs the modemsmem client. Add it in the device
tree so the modem can properly boot.

> diff --git a/arch/arm64/boot/dts/qcom/sdm670-google-common.dtsi b/arch/arm64/boot/dts/qcom/sdm670-google-common.dtsi
> index b4854801a5f5e..4604c4eb2192a 100644
> --- a/arch/arm64/boot/dts/qcom/sdm670-google-common.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm670-google-common.dtsi
> @@ -66,6 +66,11 @@ key-vol-up {
>  		};
>  	};
>  
> +	modemsmem {
> +		compatible = "google,modemsmem";
> +		qcom,smem-id = <134>;
> +	};
> +

[Severity: High]
Is there a missing probe dependency between the modemsmem platform device
and the modem remoteproc driver?

Because this node is defined as a standalone platform device with no
parent-child relationship or fw_devlink dependency (like a phandle) to the
modem node, the kernel driver core will probe them in an unconstrained order.

If remoteproc probes first and starts the modem firmware, will the modem
driver crash or fail to boot if it attempts to read SMEM item 134 before it
is allocated and populated?

[Severity: High]
Does this node addition trigger a probe failure on warm reboots?

Looking at modemsmem_probe() in drivers/soc/qcom/modemsmem.c:

	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");

If the requested SMEM item was already allocated (e.g., by the bootloader
or persisting across a warm reboot), qcom_smem_alloc() returns -EEXIST.

Because modemsmem_probe() treats any non-zero return value as a fatal error
instead of tolerating -EEXIST, will this prevent the modem from receiving
updated SoC information?

[Severity: High]
Does allocating this SMEM item expose uninitialized memory to the modem?

Looking at write_socinfo() in drivers/soc/qcom/modemsmem.c:

	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;

qcom_smem_alloc() allocates memory from the SMEM region but does not
zero-initialize it. Since only 5 of the 8 fields in struct modem_smem_info
are assigned, the remaining fields (modem_flag, efs_magic, ftm_magic) are
left untouched.

Could this lead to unpredictable behavior if the modem firmware reads and
processes these uninitialized garbage values?

>  	reserved-memory {
>  		#address-cells = <2>;
>  		#size-cells = <2>;

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

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

[sashiko-bot]

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

Re: [PATCH RFC 0/4] Add modemsmem for Google phones

[Konrad Dybcio]

On 9/6/26 2:41 AM, David Heidelberg via B4 Relay wrote:
> Q:
> Where do these types of drivers go? This driver is probably providing
> info to vendor code in the modem firmware, so it might not belong in
> drivers/soc/qcom/.
> 
> Maybe it should go in another directory, like drivers/misc or firmware?
> 
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---
> David Heidelberg (2):
>       dt-bindings: soc: google: Add modemsmem

Would a list of machine compatibles be better suited here?

Konrad