Re: [PATCH v4 1/2] soc: qcom: smem: Expose DDR data from SMEM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:

New issues:
- [High] The global pointer `__dram` is assigned memory managed by `devm_kzalloc`, but is never cleared to `NULL` upon driver removal, leading to a Use-After-Free when accessed by other drivers.
- [High] Firmware/hardware SMEM structures incorrectly use the configurable `phys_addr_t` type, breaking binary compatibility with the fixed firmware layout.
- [High] `debugfs_remove_recursive()` is called twice on the same dentry if `socinfo` registration fails, leading to a Double-Free/Use-After-Free.
- [Medium] The `__le32` field `freq_khz` is used directly in an arithmetic operation without conversion to native CPU endianness.
- [Medium] Multiplying `freq_khz` by `1000` evaluates as a 32-bit unsigned integer, which will overflow for memory frequencies above ~4.29 GHz.
- [Medium] The global `__smem` pointer is published before `smem_dram_parse()` completes, allowing concurrent callers to `qcom_smem_dram_get_hbb()` to erroneously observe a NULL `__dram`.
- [Medium] The global `__dram` pointer is written and read locklessly without necessary memory barriers, risking exposure of uninitialized struct fields to concurrent readers.

Pre-existing issues:
- [High] `platform_device_unregister()` is unconditionally called with an `ERR_PTR` if `socinfo` registration fails, leading to a crash.
--

--- Patch [1]: [PATCH v4 1/2] soc: qcom: smem: Expose DDR data from SMEM ---
commit 21c7410afeccade097a3fa15bf0554d407b561bb
Author: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

soc: qcom: smem: Expose DDR data from SMEM

Most modern Qualcomm platforms (>= SM8150) expose information about the
DDR memory present on the system via SMEM.

Details from this information is used in various scenarios, such as
multimedia drivers configuring the hardware based on the "Highest Bank
address Bit" (hbb), or the list of valid frequencies in validation
scenarios...

Add support for parsing v3-v7 version of the structs. Unforunately,
they are not versioned, so some elbow grease is necessary to determine
which one is present. See for reference:

ver 3: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/1d11897d2cfcc7b85f28ff74c445018dbbecac7a
ver 4: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/f6e9aa549260bbc0bdcb156c2b05f48dc5963203
ver 5: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/617d3297abe8b1b8dd3de3d1dd69c3961e6f343f
ver 5 with 6regions: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/d770e009f9bae58d56d926f7490bbfb45af8341f
ver 6: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/62659b557fdb1551b20fae8073d1d701dfa8a62e
ver 7: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/734d95599c5ebb1ca0d4e1639142e65c590532b7

Reviewed-by: Bjorn Andersson <andersson@kernel.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index afb21a778fe7b..4315512d3a1d4 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c

[ ... ]

> @@ -1239,17 +1244,24 @@ static int qcom_smem_probe(struct platform_device *pdev)
>  
>  	__smem = smem;
>  
> +	smem->debugfs_dir = smem_dram_parse(smem->dev);

[Severity: Medium]
Is it safe to publish the global __smem pointer before smem_dram_parse
completes?

If a concurrent driver probe calls qcom_smem_is_available, it will return
true, and the driver might then call qcom_smem_dram_get_hbb. Since the
__dram pointer hasn't been initialized yet, wouldn't the caller falsely
receive -ENODATA?

> +
>  	smem->socinfo = platform_device_register_data(&pdev->dev, "qcom-socinfo",
>  						      PLATFORM_DEVID_NONE, NULL,
>  						      0);
> -	if (IS_ERR(smem->socinfo))
> +	if (IS_ERR(smem->socinfo)) {
> +		debugfs_remove_recursive(smem->debugfs_dir);
> +

[Severity: High]
This is a pre-existing issue, but if platform_device_register_data fails, it
returns an ERR_PTR which is stored in smem->socinfo.

Since probe still returns 0 (success) in this case, when the driver is later
removed, qcom_smem_remove will pass this ERR_PTR directly to
platform_device_unregister. Could this cause a crash when the driver core
attempts to dereference it?

[Severity: High]
Additionally, when socinfo registration fails, we call
debugfs_remove_recursive on smem->debugfs_dir, but we leave the pointer
intact.

When the driver is later unbound, qcom_smem_remove calls
debugfs_remove_recursive again on __smem->debugfs_dir. Does this trigger a
double-free or use-after-free in the debugfs core?

>  		dev_dbg(&pdev->dev, "failed to register socinfo device\n");
> +	}
>  
>  	return 0;
>  }
>  
>  static void qcom_smem_remove(struct platform_device *pdev)
>  {
> +	debugfs_remove_recursive(__smem->debugfs_dir);
> +

[Severity: High]
When the driver is unbound, the devm-managed dram memory allocated in
smem_dram_parse will be freed.

However, the global __dram pointer is never cleared to NULL. If an external
driver later calls the exported qcom_smem_dram_get_hbb, will it bypass the
NULL check and dereference the freed memory?

>  	platform_device_unregister(__smem->socinfo);
>  
>  	xa_destroy(&__smem->partitions);

[ ... ]

> diff --git a/drivers/soc/qcom/smem_dramc.c b/drivers/soc/qcom/smem_dramc.c
> new file mode 100644
> index 0000000000000..2998573a6e074
> --- /dev/null
> +++ b/drivers/soc/qcom/smem_dramc.c

[ ... ]

> +/* V3 */
> +struct ddr_freq_plan_v3 {
> +	struct ddr_freq_table ddr_freq[MAX_DDR_FREQ_NUM_V3];
> +	u8 num_ddr_freqs;
> +	phys_addr_t clk_period_address;
> +};

[Severity: High]
Does using phys_addr_t in these firmware structures break binary compatibility?

Since the size of phys_addr_t depends on kernel configuration (e.g., 32-bit vs
64-bit), this could alter the structure layout and offsets. Should these use
explicit fixed-width types like u32 or u64 to match the fixed firmware ABI?
(This also appears to affect ddr_xbl2quantum_smem_data and ddr_freq_plan_v5).

[ ... ]

> +int qcom_smem_dram_get_hbb(void)
> +{
> +	if (!__dram || !__dram->hbb)
> +		return -ENODATA;

[Severity: Medium]
Are memory barriers required here?

Since smem_dram_parse writes to the global __dram pointer without
smp_store_release, and this function reads it without smp_load_acquire, could
CPU or compiler reordering cause concurrent readers to observe a non-NULL
pointer but read uninitialized values from its fields?

[ ... ]

> +static void smem_dram_parse_v3_data(struct smem_dram *dram, void *data)
> +{
> +	struct ddr_details_v3 *details = data;
> +
> +	for (int i = 0; i < MAX_DDR_FREQ_NUM_V3; i++) {
> +		struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
> +
> +		if (freq_entry->freq_khz && freq_entry->enabled) {
> +			u32 freq_khz = le32_to_cpu(freq_entry->freq_khz);
> +			dram->frequencies[dram->num_frequencies++] = 1000 * freq_khz;

[Severity: Medium]
Can this multiplication overflow on modern high-speed memory?

The expression 1000 * freq_khz uses a 32-bit unsigned integer multiplication.
If the frequency exceeds 4,294,967 kHz, the value will wrap around before
being implicitly promoted to unsigned long for the array assignment. Would it
help to cast freq_khz to unsigned long first? (This logic appears in
smem_dram_parse_v5_data as well).

> +		}
> +	}
> +}
> +
> +static void smem_dram_parse_v3_14freqs_data(struct smem_dram *dram, void *data)
> +{
> +	struct ddr_details_v3_14freqs *details = data;
> +
> +	for (int i = 0; i < MAX_DDR_FREQ_NUM_V3 + 1; i++) {
> +		struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
> +
> +		if (freq_entry->freq_khz && freq_entry->enabled)
> +			dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;

[Severity: Medium]
Is the little-endian to CPU conversion missing here?

In the other parsing functions, le32_to_cpu is used to read freq_khz. Here it
is read directly, which might result in incorrect frequency calculations on
big-endian systems.

> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-topic-smem_dramc-v4-0-748f992278a3@oss.qualcomm.com?part=1