| Message ID | 20241119-qcom-scm-missing-barriers-and-all-sort-of-srap-v1-2-7056127007a7@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | firmware: qcom: scm: Fixes for concurrency | expand |
On 19/11/2024 19:33, Krzysztof Kozlowski wrote: > 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. > > 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; I mentioned in commit msg that previous commit adds barrier in qcom_scm_get_tzmem_pool(), so to be clear: This depends on previous commit, because that barrier in qcom_scm_is_available() solves the control dependency here, assuming the minimal guarantee #1 ("On any given CPU, dependent memory accesses will be issued in order, with respect to itself.") If this is inlined by compiler it will be: scm = READ_ONCE(__scm); barrier() if (scm) return scm->mempool; else return NULL; Which should be even safer than standard guarantee above (according to my understanding). Best regards, Krzysztof
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)
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. 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(-)