Changes in v2: - Patch #2: Extend commit msg - Patch #4: Store NULL - Add Rb tags - Link to v1: https://lore.kernel.org/r/20241119-qcom-scm-missing-barriers-and-all-sort-of...
Description =========== SCM driver looks messy in terms of handling concurrency of probe. The driver exports interface which is guarded by global '__scm' variable but: 1. Lacks proper read barrier (commit adding write barriers mixed up READ_ONCE with a read barrier). 2. Lacks barriers or checks for '__scm' in multiple places. 3. Lacks probe error cleanup.
All the issues here are non-urgent, IOW, they were here for some time (v6.10-rc1 and earlier).
Best regards, Krzysztof
--- Krzysztof Kozlowski (6): firmware: qcom: scm: Fix missing read barrier in qcom_scm_is_available() firmware: qcom: scm: Fix missing read barrier in qcom_scm_get_tzmem_pool() firmware: qcom: scm: Handle various probe ordering for qcom_scm_assign_mem() firmware: qcom: scm: Cleanup global '__scm' on probe failures firmware: qcom: scm: smc: Handle missing SCM device firmware: qcom: scm: smc: Narrow 'mempool' variable scope
drivers/firmware/qcom/qcom_scm-smc.c | 6 +++- drivers/firmware/qcom/qcom_scm.c | 55 +++++++++++++++++++++++++----------- 2 files changed, 44 insertions(+), 17 deletions(-) --- base-commit: d1486dca38afd08ca279ae94eb3a397f10737824 change-id: 20241119-qcom-scm-missing-barriers-and-all-sort-of-srap-a25d59074882
Best regards,
Commit 2e4955167ec5 ("firmware: qcom: scm: Fix __scm and waitq completion variable initialization") introduced a write barrier in probe function to store global '__scm' variable. It also claimed that it added a read barrier, because as we all known barriers are paired (see memory-barriers.txt: "Note that write barriers should normally be paired with read or address-dependency barriers"), however it did not really add it.
The offending commit used READ_ONCE() to access '__scm' global which is not a barrier.
The barrier is needed so the store to '__scm' will be properly visible. This is most likely not fatal in current driver design, because missing read barrier would mean qcom_scm_is_available() callers will access old value, NULL. Driver does not support unbinding and does not correctly handle probe failures, thus there is no risk of stale or old pointer in '__scm' variable.
However for code correctness, readability and to be sure that we did not mess up something in this tricky topic of SMP barriers, add a read barrier for accessing '__scm'. Change also comment from useless/obvious what does barrier do, to what is expected: which other parts of the code are involved here.
Fixes: 2e4955167ec5 ("firmware: qcom: scm: Fix __scm and waitq completion variable initialization") Cc: stable@vger.kernel.org Reviewed-by: Bartosz Golaszewski bartosz.golaszewski@linaro.org Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org --- drivers/firmware/qcom/qcom_scm.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c index 72bf87ddcd969834609cda2aa915b67505e93943..246d672e8f7f0e2a326a03a5af40cd434a665e67 100644 --- a/drivers/firmware/qcom/qcom_scm.c +++ b/drivers/firmware/qcom/qcom_scm.c @@ -1867,7 +1867,8 @@ static int qcom_scm_qseecom_init(struct qcom_scm *scm) */ bool qcom_scm_is_available(void) { - return !!READ_ONCE(__scm); + /* Paired with smp_store_release() in qcom_scm_probe */ + return !!smp_load_acquire(&__scm); } EXPORT_SYMBOL_GPL(qcom_scm_is_available);
@@ -2024,7 +2025,7 @@ static int qcom_scm_probe(struct platform_device *pdev) if (ret) return ret;
- /* Let all above stores be available after this */ + /* Paired with smp_load_acquire() in qcom_scm_is_available(). */ smp_store_release(&__scm, scm);
irq = platform_get_irq_optional(pdev, 0);
Commit 2e4955167ec5 ("firmware: qcom: scm: Fix __scm and waitq completion variable initialization") introduced a write barrier in probe function to store global '__scm' variable. We all known barriers are paired (see memory-barriers.txt: "Note that write barriers should normally be paired with read or address-dependency barriers"), therefore accessing it from concurrent contexts requires read barrier. Previous commit added such barrier in qcom_scm_is_available(), so let's use that directly.
Lack of this read barrier can result in fetching stale '__scm' variable value, NULL, and dereferencing it.
Note that barrier in qcom_scm_is_available() satisfies here the control dependency.
Fixes: ca61d6836e6f ("firmware: qcom: scm: fix a NULL-pointer dereference") Fixes: 449d0d84bcd8 ("firmware: qcom: scm: smc: switch to using the SCM allocator") Cc: stable@vger.kernel.org Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org --- drivers/firmware/qcom/qcom_scm.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c index 246d672e8f7f0e2a326a03a5af40cd434a665e67..5d91b8e22844608f35432f1ba9c08d477d4ff762 100644 --- a/drivers/firmware/qcom/qcom_scm.c +++ b/drivers/firmware/qcom/qcom_scm.c @@ -217,7 +217,10 @@ static DEFINE_SPINLOCK(scm_query_lock);
struct qcom_tzmem_pool *qcom_scm_get_tzmem_pool(void) { - return __scm ? __scm->mempool : NULL; + if (!qcom_scm_is_available()) + return NULL; + + return __scm->mempool; }
static enum qcom_scm_convention __get_convention(void)
On Mon, 09 Dec 2024 15:27:53 +0100, Krzysztof Kozlowski wrote:
Changes in v2:
- Patch #2: Extend commit msg
- Patch #4: Store NULL
- Add Rb tags
- Link to v1: https://lore.kernel.org/r/20241119-qcom-scm-missing-barriers-and-all-sort-of...
Description
SCM driver looks messy in terms of handling concurrency of probe. The driver exports interface which is guarded by global '__scm' variable but:
- Lacks proper read barrier (commit adding write barriers mixed up READ_ONCE with a read barrier).
- Lacks barriers or checks for '__scm' in multiple places.
- Lacks probe error cleanup.
[...]
Applied, thanks!
[1/6] firmware: qcom: scm: Fix missing read barrier in qcom_scm_is_available() commit: 0a744cceebd0480cb39587b3b1339d66a9d14063 [2/6] firmware: qcom: scm: Fix missing read barrier in qcom_scm_get_tzmem_pool() commit: b628510397b5cafa1f5d3e848a28affd1c635302 [4/6] firmware: qcom: scm: Cleanup global '__scm' on probe failures commit: 1e76b546e6fca7eb568161f408133904ca6bcf4f [5/6] firmware: qcom: scm: smc: Handle missing SCM device commit: 94f48ecf0a538019ca2025e0b0da391f8e7cc58c [6/6] firmware: qcom: scm: smc: Narrow 'mempool' variable scope commit: a4332f6c791e1d70bf025ac51afa968607b9812b
Best regards,
linux-stable-mirror@lists.linaro.org
-
Bjorn Andersson -
Krzysztof Kozlowski