[PATCH 0/2] qcom_tzmem: Enhance Error Handling for shmbridge

[PATCH 0/2] qcom_tzmem: Enhance Error Handling for shmbridge

[Kuldeep Singh]

This patchset addresses the tzmem driver probe failure caused by
incorrect error handling. The qcom_scm_shm_bridge_enable() SCM call
captures SCM success/failure in a0 and E_NOT_SUPPORTED in a1.

Previously, qcom_scm returned values based solely on a0, without
capturing not_supported scenario. This patchset corrects that behavior.

Along with this, add sanity checks on input parameters passed to exposed
APIs as it is missing currently.

Kuldeep Singh (1):
  firmware: qcom: qcom_tzmem: Implement sanity checks

Qingqing Zhou (1):
  firmware: qcom: scm: Return -EOPNOTSUPP for unsupported SHM bridge
    enabling

 drivers/firmware/qcom/qcom_scm.c   | 12 +++++++++++-
 drivers/firmware/qcom/qcom_tzmem.c | 17 ++++++++++++++++-
 2 files changed, 27 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/2] firmware: qcom: scm: Return -EOPNOTSUPP for unsupported SHM bridge enabling

[Kuldeep Singh]

From: Qingqing Zhou <quic_qqzhou@quicinc.com>

Currently for enabling shm bridge, QTEE will return 0 and put error 4 into
result[0] to qcom_scm for unsupported platform, tzmem will consider this
as an unknown error not the unsupported case on the platform.

Error log:
[    0.177224] qcom_scm firmware:scm: error (____ptrval____): Failed to enable the TrustZone memory allocator
[    0.177244] qcom_scm firmware:scm: probe with driver qcom_scm failed with error 4

Change the function call qcom_scm_shm_bridge_enable() to remap this
result[0] into the unsupported error and then tzmem can consider this as
unsupported case instead of reporting an error.

Signed-off-by: Qingqing Zhou <quic_qqzhou@quicinc.com>
Co-developed-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
Signed-off-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
---
 drivers/firmware/qcom/qcom_scm.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 10986cb11ec0..620313359042 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -111,6 +111,10 @@ enum qcom_scm_qseecom_tz_cmd_info {
 	QSEECOM_TZ_CMD_INFO_VERSION		= 3,
 };
 
+enum qcom_scm_shm_bridge_result {
+	SHMBRIDGE_RESULT_NOTSUPP	= 4,
+};
+
 #define QSEECOM_MAX_APP_NAME_SIZE		64
 
 /* Each bit configures cold/warm boot address for one of the 4 CPUs */
@@ -1361,6 +1365,8 @@ EXPORT_SYMBOL_GPL(qcom_scm_lmh_dcvsh_available);
 
 int qcom_scm_shm_bridge_enable(void)
 {
+	int ret;
+
 	struct qcom_scm_desc desc = {
 		.svc = QCOM_SCM_SVC_MP,
 		.cmd = QCOM_SCM_MP_SHM_BRIDGE_ENABLE,
@@ -1373,7 +1379,11 @@ int qcom_scm_shm_bridge_enable(void)
 					  QCOM_SCM_MP_SHM_BRIDGE_ENABLE))
 		return -EOPNOTSUPP;
 
-	return qcom_scm_call(__scm->dev, &desc, &res) ?: res.result[0];
+	ret = qcom_scm_call(__scm->dev, &desc, &res);
+	if (!ret && res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
+		return -EOPNOTSUPP;
+
+	return ret ?: res.result[0];
 }
 EXPORT_SYMBOL_GPL(qcom_scm_shm_bridge_enable);
 
-- 
2.34.1

[PATCH 2/2] firmware: qcom: qcom_tzmem: Implement sanity checks

[Kuldeep Singh]

The qcom_tzmem driver currently has multiple exposed APIs that lack
validations on input parameters. This oversight can lead to unexpected
crashes due to null pointer dereference when incorrect inputs are
provided.

To address this issue, add required sanity for all input parameters in
the exposed APIs.

Signed-off-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
---
 drivers/firmware/qcom/qcom_tzmem.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c
index 92b365178235..2f2e1f2fa9fc 100644
--- a/drivers/firmware/qcom/qcom_tzmem.c
+++ b/drivers/firmware/qcom/qcom_tzmem.c
@@ -203,6 +203,9 @@ qcom_tzmem_pool_new(const struct qcom_tzmem_pool_config *config)
 
 	might_sleep();
 
+	if (!config || !config->policy)
+		return ERR_PTR(-EINVAL);
+
 	switch (config->policy) {
 	case QCOM_TZMEM_POLICY_STATIC:
 		if (!config->initial_size)
@@ -316,6 +319,9 @@ devm_qcom_tzmem_pool_new(struct device *dev,
 	struct qcom_tzmem_pool *pool;
 	int ret;
 
+	if (!dev || !config)
+		return ERR_PTR(-EINVAL);
+
 	pool = qcom_tzmem_pool_new(config);
 	if (IS_ERR(pool))
 		return pool;
@@ -366,7 +372,7 @@ void *qcom_tzmem_alloc(struct qcom_tzmem_pool *pool, size_t size, gfp_t gfp)
 	unsigned long vaddr;
 	int ret;
 
-	if (!size)
+	if (!pool || !size)
 		return NULL;
 
 	size = PAGE_ALIGN(size);
@@ -412,6 +418,9 @@ void qcom_tzmem_free(void *vaddr)
 {
 	struct qcom_tzmem_chunk *chunk;
 
+	if (!vaddr)
+		return;
+
 	scoped_guard(spinlock_irqsave, &qcom_tzmem_chunks_lock)
 		chunk = radix_tree_delete_item(&qcom_tzmem_chunks,
 					       (unsigned long)vaddr, NULL);
@@ -446,6 +455,9 @@ phys_addr_t qcom_tzmem_to_phys(void *vaddr)
 	void __rcu **slot;
 	phys_addr_t ret;
 
+	if (!vaddr)
+		return 0;
+
 	guard(spinlock_irqsave)(&qcom_tzmem_chunks_lock);
 
 	radix_tree_for_each_slot(slot, &qcom_tzmem_chunks, &iter, 0) {
@@ -466,6 +478,9 @@ EXPORT_SYMBOL_GPL(qcom_tzmem_to_phys);
 
 int qcom_tzmem_enable(struct device *dev)
 {
+	if (!dev)
+		return -EINVAL;
+
 	if (qcom_tzmem_dev)
 		return -EBUSY;
 
-- 
2.34.1

Re: [PATCH 1/2] firmware: qcom: scm: Return -EOPNOTSUPP for unsupported SHM bridge enabling

[Dmitry Baryshkov]

On Sat, Oct 05, 2024 at 07:31:49PM GMT, Kuldeep Singh wrote:
> From: Qingqing Zhou <quic_qqzhou@quicinc.com>
> 
> Currently for enabling shm bridge, QTEE will return 0 and put error 4 into
> result[0] to qcom_scm for unsupported platform, tzmem will consider this
> as an unknown error not the unsupported case on the platform.
> 
> Error log:
> [    0.177224] qcom_scm firmware:scm: error (____ptrval____): Failed to enable the TrustZone memory allocator
> [    0.177244] qcom_scm firmware:scm: probe with driver qcom_scm failed with error 4
> 
> Change the function call qcom_scm_shm_bridge_enable() to remap this
> result[0] into the unsupported error and then tzmem can consider this as
> unsupported case instead of reporting an error.
> 
> Signed-off-by: Qingqing Zhou <quic_qqzhou@quicinc.com>
> Co-developed-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
> Signed-off-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 10986cb11ec0..620313359042 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -111,6 +111,10 @@ enum qcom_scm_qseecom_tz_cmd_info {
>  	QSEECOM_TZ_CMD_INFO_VERSION		= 3,
>  };
>  
> +enum qcom_scm_shm_bridge_result {
> +	SHMBRIDGE_RESULT_NOTSUPP	= 4,
> +};
> +
>  #define QSEECOM_MAX_APP_NAME_SIZE		64
>  
>  /* Each bit configures cold/warm boot address for one of the 4 CPUs */
> @@ -1361,6 +1365,8 @@ EXPORT_SYMBOL_GPL(qcom_scm_lmh_dcvsh_available);
>  
>  int qcom_scm_shm_bridge_enable(void)
>  {
> +	int ret;
> +
>  	struct qcom_scm_desc desc = {
>  		.svc = QCOM_SCM_SVC_MP,
>  		.cmd = QCOM_SCM_MP_SHM_BRIDGE_ENABLE,
> @@ -1373,7 +1379,11 @@ int qcom_scm_shm_bridge_enable(void)
>  					  QCOM_SCM_MP_SHM_BRIDGE_ENABLE))
>  		return -EOPNOTSUPP;
>  
> -	return qcom_scm_call(__scm->dev, &desc, &res) ?: res.result[0];
> +	ret = qcom_scm_call(__scm->dev, &desc, &res);
> +	if (!ret && res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
> +		return -EOPNOTSUPP;
> +
> +	return ret ?: res.result[0];

Could you please make it less cryptic?

if (ret)
	return ret;

if (res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
	return -EOPNOTSUPP;

return res.result[0];

>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_shm_bridge_enable);
>  
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] firmware: qcom: qcom_tzmem: Implement sanity checks

[Dmitry Baryshkov]

On Sat, Oct 05, 2024 at 07:31:50PM GMT, Kuldeep Singh wrote:
> The qcom_tzmem driver currently has multiple exposed APIs that lack
> validations on input parameters. This oversight can lead to unexpected
> crashes due to null pointer dereference when incorrect inputs are
> provided.
> 
> To address this issue, add required sanity for all input parameters in
> the exposed APIs.

Please don't be overprotective. Inserting guarding conditions is good,
inserting useless guarding conditions is bad, it complicates the driver
and makes it harder to follow. Please validate return data rather than
adding extra checks to the functions.

> 
> Signed-off-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
> ---
>  drivers/firmware/qcom/qcom_tzmem.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c
> index 92b365178235..2f2e1f2fa9fc 100644
> --- a/drivers/firmware/qcom/qcom_tzmem.c
> +++ b/drivers/firmware/qcom/qcom_tzmem.c
> @@ -203,6 +203,9 @@ qcom_tzmem_pool_new(const struct qcom_tzmem_pool_config *config)
>  
>  	might_sleep();
>  
> +	if (!config || !config->policy)

config can not be NULL
Ack for config->policy check.

> +		return ERR_PTR(-EINVAL);
> +
>  	switch (config->policy) {
>  	case QCOM_TZMEM_POLICY_STATIC:
>  		if (!config->initial_size)
> @@ -316,6 +319,9 @@ devm_qcom_tzmem_pool_new(struct device *dev,
>  	struct qcom_tzmem_pool *pool;
>  	int ret;
>  
> +	if (!dev || !config)
> +		return ERR_PTR(-EINVAL);

dev can not be NULL
config can not be NULL

> +
>  	pool = qcom_tzmem_pool_new(config);
>  	if (IS_ERR(pool))
>  		return pool;
> @@ -366,7 +372,7 @@ void *qcom_tzmem_alloc(struct qcom_tzmem_pool *pool, size_t size, gfp_t gfp)
>  	unsigned long vaddr;
>  	int ret;
>  
> -	if (!size)
> +	if (!pool || !size)

Is it really possible to pass NULL as pool? Which code path leads to
this event?

>  		return NULL;
>  
>  	size = PAGE_ALIGN(size);
> @@ -412,6 +418,9 @@ void qcom_tzmem_free(void *vaddr)
>  {
>  	struct qcom_tzmem_chunk *chunk;
>  
> +	if (!vaddr)
> +		return;

Ack, simplifies error handling and matches existing kfree-like functions.

> +
>  	scoped_guard(spinlock_irqsave, &qcom_tzmem_chunks_lock)
>  		chunk = radix_tree_delete_item(&qcom_tzmem_chunks,
>  					       (unsigned long)vaddr, NULL);
> @@ -446,6 +455,9 @@ phys_addr_t qcom_tzmem_to_phys(void *vaddr)
>  	void __rcu **slot;
>  	phys_addr_t ret;
>  
> +	if (!vaddr)

Is it possible?

> +		return 0;
> +
>  	guard(spinlock_irqsave)(&qcom_tzmem_chunks_lock);
>  
>  	radix_tree_for_each_slot(slot, &qcom_tzmem_chunks, &iter, 0) {
> @@ -466,6 +478,9 @@ EXPORT_SYMBOL_GPL(qcom_tzmem_to_phys);
>  
>  int qcom_tzmem_enable(struct device *dev)
>  {
> +	if (!dev)
> +		return -EINVAL;

Definitely not possible.

> +
>  	if (qcom_tzmem_dev)
>  		return -EBUSY;
>  
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/2] firmware: qcom: scm: Return -EOPNOTSUPP for unsupported SHM bridge enabling

[Kuldeep Singh]

>>  int qcom_scm_shm_bridge_enable(void)
>>  {
>> +	int ret;
>> +
>>  	struct qcom_scm_desc desc = {
>>  		.svc = QCOM_SCM_SVC_MP,
>>  		.cmd = QCOM_SCM_MP_SHM_BRIDGE_ENABLE,
>> @@ -1373,7 +1379,11 @@ int qcom_scm_shm_bridge_enable(void)
>>  					  QCOM_SCM_MP_SHM_BRIDGE_ENABLE))
>>  		return -EOPNOTSUPP;
>>  
>> -	return qcom_scm_call(__scm->dev, &desc, &res) ?: res.result[0];
>> +	ret = qcom_scm_call(__scm->dev, &desc, &res);
>> +	if (!ret && res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
>> +		return -EOPNOTSUPP;
>> +
>> +	return ret ?: res.result[0];
> 
> Could you please make it less cryptic?
> 
> if (ret)
> 	return ret;
> 
> if (res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
> 	return -EOPNOTSUPP;
> 
> return res.result[0];

Sure Dmitry, this looks more cleaner.
Will update in next rev.

-- 
Regards
Kuldeep

Re: [PATCH 2/2] firmware: qcom: qcom_tzmem: Implement sanity checks

[Bjorn Andersson]

On Sat, Oct 05, 2024 at 07:31:50PM GMT, Kuldeep Singh wrote:
> The qcom_tzmem driver currently has multiple exposed APIs that lack
> validations on input parameters. This oversight can lead to unexpected
> crashes due to null pointer dereference when incorrect inputs are
> provided.
> 
> To address this issue, add required sanity for all input parameters in
> the exposed APIs.
> 

Unless there's good reason for the opposite, I rather see that we define
the API to only accept valid pointers. Then if a client passes a NULL we
get a oops with a nice callstack, which is easy to debug.

The alternative is that we return -EINVAL, which not unlikely is
propagated to some application which may or may not result in a bug
report from a user - without any tangible information about where things
went wrong.

But, if you think there's a good reason, please let me know.

Regards,
Bjorn

> Signed-off-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
> ---
>  drivers/firmware/qcom/qcom_tzmem.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c
> index 92b365178235..2f2e1f2fa9fc 100644
> --- a/drivers/firmware/qcom/qcom_tzmem.c
> +++ b/drivers/firmware/qcom/qcom_tzmem.c
> @@ -203,6 +203,9 @@ qcom_tzmem_pool_new(const struct qcom_tzmem_pool_config *config)
>  
>  	might_sleep();
>  
> +	if (!config || !config->policy)
> +		return ERR_PTR(-EINVAL);
> +
>  	switch (config->policy) {
>  	case QCOM_TZMEM_POLICY_STATIC:
>  		if (!config->initial_size)
> @@ -316,6 +319,9 @@ devm_qcom_tzmem_pool_new(struct device *dev,
>  	struct qcom_tzmem_pool *pool;
>  	int ret;
>  
> +	if (!dev || !config)
> +		return ERR_PTR(-EINVAL);
> +
>  	pool = qcom_tzmem_pool_new(config);
>  	if (IS_ERR(pool))
>  		return pool;
> @@ -366,7 +372,7 @@ void *qcom_tzmem_alloc(struct qcom_tzmem_pool *pool, size_t size, gfp_t gfp)
>  	unsigned long vaddr;
>  	int ret;
>  
> -	if (!size)
> +	if (!pool || !size)
>  		return NULL;
>  
>  	size = PAGE_ALIGN(size);
> @@ -412,6 +418,9 @@ void qcom_tzmem_free(void *vaddr)
>  {
>  	struct qcom_tzmem_chunk *chunk;
>  
> +	if (!vaddr)
> +		return;
> +
>  	scoped_guard(spinlock_irqsave, &qcom_tzmem_chunks_lock)
>  		chunk = radix_tree_delete_item(&qcom_tzmem_chunks,
>  					       (unsigned long)vaddr, NULL);
> @@ -446,6 +455,9 @@ phys_addr_t qcom_tzmem_to_phys(void *vaddr)
>  	void __rcu **slot;
>  	phys_addr_t ret;
>  
> +	if (!vaddr)
> +		return 0;
> +
>  	guard(spinlock_irqsave)(&qcom_tzmem_chunks_lock);
>  
>  	radix_tree_for_each_slot(slot, &qcom_tzmem_chunks, &iter, 0) {
> @@ -466,6 +478,9 @@ EXPORT_SYMBOL_GPL(qcom_tzmem_to_phys);
>  
>  int qcom_tzmem_enable(struct device *dev)
>  {
> +	if (!dev)
> +		return -EINVAL;
> +
>  	if (qcom_tzmem_dev)
>  		return -EBUSY;
>  
> -- 
> 2.34.1
> 

Re: [PATCH 1/2] firmware: qcom: scm: Return -EOPNOTSUPP for unsupported SHM bridge enabling

[Bjorn Andersson]

On Sat, Oct 05, 2024 at 07:31:49PM GMT, Kuldeep Singh wrote:

Please shorten the subject a bit, perhaps:
"firmware: qcom: scm: Improve unsupported SHM bridge detection"

> From: Qingqing Zhou <quic_qqzhou@quicinc.com>
> 
> Currently for enabling shm bridge, QTEE will return 0 and put error 4 into

s/for/when/

> result[0] to qcom_scm for unsupported platform, tzmem will consider this
> as an unknown error not the unsupported case on the platform.
> 
> Error log:
> [    0.177224] qcom_scm firmware:scm: error (____ptrval____): Failed to enable the TrustZone memory allocator
> [    0.177244] qcom_scm firmware:scm: probe with driver qcom_scm failed with error 4
> 
> Change the function call qcom_scm_shm_bridge_enable() to remap this
> result[0] into the unsupported error and then tzmem can consider this as
> unsupported case instead of reporting an error.
> 

Sounds like we want a Fixes tag here.

> Signed-off-by: Qingqing Zhou <quic_qqzhou@quicinc.com>
> Co-developed-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
> Signed-off-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 10986cb11ec0..620313359042 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -111,6 +111,10 @@ enum qcom_scm_qseecom_tz_cmd_info {
>  	QSEECOM_TZ_CMD_INFO_VERSION		= 3,
>  };
>  
> +enum qcom_scm_shm_bridge_result {
> +	SHMBRIDGE_RESULT_NOTSUPP	= 4,
> +};

This is not an enumeration, but a fixed defined constant. Please use
#define.

> +
>  #define QSEECOM_MAX_APP_NAME_SIZE		64
>  
>  /* Each bit configures cold/warm boot address for one of the 4 CPUs */
> @@ -1361,6 +1365,8 @@ EXPORT_SYMBOL_GPL(qcom_scm_lmh_dcvsh_available);
>  
>  int qcom_scm_shm_bridge_enable(void)
>  {
> +	int ret;
> +
>  	struct qcom_scm_desc desc = {
>  		.svc = QCOM_SCM_SVC_MP,
>  		.cmd = QCOM_SCM_MP_SHM_BRIDGE_ENABLE,
> @@ -1373,7 +1379,11 @@ int qcom_scm_shm_bridge_enable(void)
>  					  QCOM_SCM_MP_SHM_BRIDGE_ENABLE))
>  		return -EOPNOTSUPP;
>  
> -	return qcom_scm_call(__scm->dev, &desc, &res) ?: res.result[0];
> +	ret = qcom_scm_call(__scm->dev, &desc, &res);
> +	if (!ret && res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
> +		return -EOPNOTSUPP;
> +
> +	return ret ?: res.result[0];

I'd prefer, with the additional check, that you'd structure it like this:

	if (ret)
		return ret;

	if (res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
		return -EOPNOTSUPP;

	return res.result[0];


That way we deal with SCM-call errors first, otherwise we inspect and
act on the returned data.

That said, the return value of this function, if non-zero, will trickle
back to and be returned from qcom_scm_probe(), where Linux expects to
see a valid error code. Are there any other result[0] values we should
handle, which would allow us to end this function with "return 0"?

Regards,
Bjorn

>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_shm_bridge_enable);
>  
> -- 
> 2.34.1
> 

Re: [PATCH 2/2] firmware: qcom: qcom_tzmem: Implement sanity checks

[Bartosz Golaszewski]

On Mon, 7 Oct 2024 at 03:18, Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Sat, Oct 05, 2024 at 07:31:50PM GMT, Kuldeep Singh wrote:
> > The qcom_tzmem driver currently has multiple exposed APIs that lack
> > validations on input parameters. This oversight can lead to unexpected
> > crashes due to null pointer dereference when incorrect inputs are
> > provided.
> >
> > To address this issue, add required sanity for all input parameters in
> > the exposed APIs.
> >
>
> Unless there's good reason for the opposite, I rather see that we define
> the API to only accept valid pointers. Then if a client passes a NULL we
> get a oops with a nice callstack, which is easy to debug.
>
> The alternative is that we return -EINVAL, which not unlikely is
> propagated to some application which may or may not result in a bug
> report from a user - without any tangible information about where things
> went wrong.

Agreed, I don't think this is a good pattern in a kernel API (as
opposed to user-space interfaces where we validate everything). We
expect a certain level of sanity from in-kernel users.

Bart

Re: [PATCH 0/2] qcom_tzmem: Enhance Error Handling for shmbridge

[Bartosz Golaszewski]

On Sat, 5 Oct 2024 at 16:02, Kuldeep Singh <quic_kuldsing@quicinc.com> wrote:
>
> This patchset addresses the tzmem driver probe failure caused by
> incorrect error handling. The qcom_scm_shm_bridge_enable() SCM call
> captures SCM success/failure in a0 and E_NOT_SUPPORTED in a1.
>
> Previously, qcom_scm returned values based solely on a0, without
> capturing not_supported scenario. This patchset corrects that behavior.
>

Ah, I guess this may be the reason for the SHM bridge enablement to
seemingly work on certain platforms and then lead to crashes when we
actually try to use it?

Bart

Re: [PATCH 1/2] firmware: qcom: scm: Return -EOPNOTSUPP for unsupported SHM bridge enabling

[Mukesh Ojha]

On Sun, Oct 06, 2024 at 08:35:57PM +0300, Dmitry Baryshkov wrote:
> On Sat, Oct 05, 2024 at 07:31:49PM GMT, Kuldeep Singh wrote:
> > From: Qingqing Zhou <quic_qqzhou@quicinc.com>
> > 
> > Currently for enabling shm bridge, QTEE will return 0 and put error 4 into
> > result[0] to qcom_scm for unsupported platform, tzmem will consider this
> > as an unknown error not the unsupported case on the platform.
> > 
> > Error log:
> > [    0.177224] qcom_scm firmware:scm: error (____ptrval____): Failed to enable the TrustZone memory allocator
> > [    0.177244] qcom_scm firmware:scm: probe with driver qcom_scm failed with error 4
> > 
> > Change the function call qcom_scm_shm_bridge_enable() to remap this
> > result[0] into the unsupported error and then tzmem can consider this as
> > unsupported case instead of reporting an error.
> > 
> > Signed-off-by: Qingqing Zhou <quic_qqzhou@quicinc.com>
> > Co-developed-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
> > Signed-off-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
> > ---
> >  drivers/firmware/qcom/qcom_scm.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> > index 10986cb11ec0..620313359042 100644
> > --- a/drivers/firmware/qcom/qcom_scm.c
> > +++ b/drivers/firmware/qcom/qcom_scm.c
> > @@ -111,6 +111,10 @@ enum qcom_scm_qseecom_tz_cmd_info {
> >  	QSEECOM_TZ_CMD_INFO_VERSION		= 3,
> >  };
> >  
> > +enum qcom_scm_shm_bridge_result {
> > +	SHMBRIDGE_RESULT_NOTSUPP	= 4,
> > +};
> > +
> >  #define QSEECOM_MAX_APP_NAME_SIZE		64
> >  
> >  /* Each bit configures cold/warm boot address for one of the 4 CPUs */
> > @@ -1361,6 +1365,8 @@ EXPORT_SYMBOL_GPL(qcom_scm_lmh_dcvsh_available);
> >  
> >  int qcom_scm_shm_bridge_enable(void)
> >  {
> > +	int ret;
> > +
> >  	struct qcom_scm_desc desc = {
> >  		.svc = QCOM_SCM_SVC_MP,
> >  		.cmd = QCOM_SCM_MP_SHM_BRIDGE_ENABLE,
> > @@ -1373,7 +1379,11 @@ int qcom_scm_shm_bridge_enable(void)
> >  					  QCOM_SCM_MP_SHM_BRIDGE_ENABLE))
> >  		return -EOPNOTSUPP;
> >  
> > -	return qcom_scm_call(__scm->dev, &desc, &res) ?: res.result[0];
> > +	ret = qcom_scm_call(__scm->dev, &desc, &res);
> > +	if (!ret && res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
> > +		return -EOPNOTSUPP;
> > +
> > +	return ret ?: res.result[0];
> 
> Could you please make it less cryptic?
> 
> if (ret)
> 	return ret;
> 
> if (res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
> 	return -EOPNOTSUPP;
> 
> return res.result[0];

Ack. for this.

-Mukesh
> 
> >  }
> >  EXPORT_SYMBOL_GPL(qcom_scm_shm_bridge_enable);
> >  
> > -- 
> > 2.34.1
> > 
> 
> -- 
> With best wishes
> Dmitry
> 

Re: [PATCH 2/2] firmware: qcom: qcom_tzmem: Implement sanity checks

[Kuldeep Singh]

On 10/7/2024 1:00 AM, Dmitry Baryshkov wrote:
> On Sat, Oct 05, 2024 at 07:31:50PM GMT, Kuldeep Singh wrote:
>> The qcom_tzmem driver currently has multiple exposed APIs that lack
>> validations on input parameters. This oversight can lead to unexpected
>> crashes due to null pointer dereference when incorrect inputs are
>> provided.
>>
>> To address this issue, add required sanity for all input parameters in
>> the exposed APIs.
> 
> Please don't be overprotective. Inserting guarding conditions is good,
> inserting useless guarding conditions is bad, it complicates the driver
> and makes it harder to follow. Please validate return data rather than
> adding extra checks to the functions.

Sure, I’ll remove the redundant checks.
Please see below for explanations.

My intention here is to handle erroneous conditions gracefully to avoid system crashes, as crashes can be detrimental.

>>
>> Signed-off-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
>> ---
>>  drivers/firmware/qcom/qcom_tzmem.c | 17 ++++++++++++++++-
>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c
>> index 92b365178235..2f2e1f2fa9fc 100644
>> --- a/drivers/firmware/qcom/qcom_tzmem.c
>> +++ b/drivers/firmware/qcom/qcom_tzmem.c
>> @@ -203,6 +203,9 @@ qcom_tzmem_pool_new(const struct qcom_tzmem_pool_config *config)
>>  
>>  	might_sleep();
>>  
>> +	if (!config || !config->policy)
> 
> config can not be NULL
> Ack for config->policy check.

Considering a scenario where user doesn't fill config struct details and call devm_qcom_tzmem_pool_new.
config will be null in that case.

> 
>> +		return ERR_PTR(-EINVAL);
>> +
>>  	switch (config->policy) {
>>  	case QCOM_TZMEM_POLICY_STATIC:
>>  		if (!config->initial_size)
>> @@ -316,6 +319,9 @@ devm_qcom_tzmem_pool_new(struct device *dev,
>>  	struct qcom_tzmem_pool *pool;
>>  	int ret;
>>  
>> +	if (!dev || !config)
>> +		return ERR_PTR(-EINVAL);
> 
> dev can not be NULL
> config can not be NULL

dev may not be always __scm->dev.
For ex: qcom_qseecom_uefisecapp.c pass it's own dev.
If new calling driver pass dev as null, will lead to NPD.

> 
>> +
>>  	pool = qcom_tzmem_pool_new(config);
>>  	if (IS_ERR(pool))
>>  		return pool;
>> @@ -366,7 +372,7 @@ void *qcom_tzmem_alloc(struct qcom_tzmem_pool *pool, size_t size, gfp_t gfp)
>>  	unsigned long vaddr;
>>  	int ret;
>>  
>> -	if (!size)
>> +	if (!pool || !size)
> 
> Is it really possible to pass NULL as pool? Which code path leads to
> this event?

qcom_tzmem_alloc/free need to be used once pool is already created with devm_qcom_tzmem_pool_new API.
If pool isn't created, then calling qcom_tzmem_alloc/free will be erroneus.

> 
>>  		return NULL;
>>  
>>  	size = PAGE_ALIGN(size);
>> @@ -412,6 +418,9 @@ void qcom_tzmem_free(void *vaddr)
>>  {
>>  	struct qcom_tzmem_chunk *chunk;
>>  
>> +	if (!vaddr)
>> +		return;
> 
> Ack, simplifies error handling and matches existing kfree-like functions.
> 
>> +
>>  	scoped_guard(spinlock_irqsave, &qcom_tzmem_chunks_lock)
>>  		chunk = radix_tree_delete_item(&qcom_tzmem_chunks,
>>  					       (unsigned long)vaddr, NULL);
>> @@ -446,6 +455,9 @@ phys_addr_t qcom_tzmem_to_phys(void *vaddr)
>>  	void __rcu **slot;
>>  	phys_addr_t ret;
>>  
>> +	if (!vaddr)
> 
> Is it possible?

Yes, A scenario where qcom_tzmem_alloc fails resulting vaddr as 0 followed by no null check.
Now, immediately passing vaddr to qcom_tzmem_to_phys will again cause NPD.

> 
>> +		return 0;
>> +
>>  	guard(spinlock_irqsave)(&qcom_tzmem_chunks_lock);
>>  
>>  	radix_tree_for_each_slot(slot, &qcom_tzmem_chunks, &iter, 0) {
>> @@ -466,6 +478,9 @@ EXPORT_SYMBOL_GPL(qcom_tzmem_to_phys);
>>  
>>  int qcom_tzmem_enable(struct device *dev)
>>  {
>> +	if (!dev)
>> +		return -EINVAL;
> 
> Definitely not possible.

Ack, by this time __scm->dev will be initialised in qcom_scm driver and cannot be null.
If some other caller even try and qcom_tzmem_dev is already set hence, return -EBUSY.
Will drop the check.

-- 
Regards
Kuldeep

Re: [PATCH 2/2] firmware: qcom: qcom_tzmem: Implement sanity checks

[Kuldeep Singh]

On 10/7/2024 7:53 PM, Bartosz Golaszewski wrote:
> On Mon, 7 Oct 2024 at 03:18, Bjorn Andersson <andersson@kernel.org> wrote:
>>
>> On Sat, Oct 05, 2024 at 07:31:50PM GMT, Kuldeep Singh wrote:
>>> The qcom_tzmem driver currently has multiple exposed APIs that lack
>>> validations on input parameters. This oversight can lead to unexpected
>>> crashes due to null pointer dereference when incorrect inputs are
>>> provided.
>>>
>>> To address this issue, add required sanity for all input parameters in
>>> the exposed APIs.
>>>
>>
>> Unless there's good reason for the opposite, I rather see that we define
>> the API to only accept valid pointers. Then if a client passes a NULL we
>> get a oops with a nice callstack, which is easy to debug>>
>> The alternative is that we return -EINVAL, which not unlikely is
>> propagated to some application which may or may not result in a bug
>> report from a user - without any tangible information about where things
>> went wrong.

Discussing with Dmitry as well on other thread over same point.
Not all checks are needed but I believe some sanity is still needed to avoid crashes.

> 
> Agreed, I don't think this is a good pattern in a kernel API (as
> opposed to user-space interfaces where we validate everything). We
> expect a certain level of sanity from in-kernel users.
> 
> Bart

-- 
Regards
Kuldeep

Re: [PATCH 0/2] qcom_tzmem: Enhance Error Handling for shmbridge

[Kuldeep Singh]

On 10/7/2024 7:55 PM, Bartosz Golaszewski wrote:
> On Sat, 5 Oct 2024 at 16:02, Kuldeep Singh <quic_kuldsing@quicinc.com> wrote:
>>
>> This patchset addresses the tzmem driver probe failure caused by
>> incorrect error handling. The qcom_scm_shm_bridge_enable() SCM call
>> captures SCM success/failure in a0 and E_NOT_SUPPORTED in a1.
>>
>> Previously, qcom_scm returned values based solely on a0, without
>> capturing not_supported scenario. This patchset corrects that behavior.
>>
> 
> Ah, I guess this may be the reason for the SHM bridge enablement to
> seemingly work on certain platforms and then lead to crashes when we
> actually try to use it?

This patchset corrects the behavior for handling unsupported SHM bridge scenarios.
If the SHM bridge is supported and enabled for a target, any subsequent failures
should be investigated to understand what went wrong.
I am willing to put effort in that case.

-- 
Regards
Kuldeep

Re: [PATCH 2/2] firmware: qcom: qcom_tzmem: Implement sanity checks

[Dmitry Baryshkov]

On Mon, 7 Oct 2024 at 21:17, Kuldeep Singh <quic_kuldsing@quicinc.com> wrote:
>
>
> On 10/7/2024 1:00 AM, Dmitry Baryshkov wrote:
> > On Sat, Oct 05, 2024 at 07:31:50PM GMT, Kuldeep Singh wrote:
> >> The qcom_tzmem driver currently has multiple exposed APIs that lack
> >> validations on input parameters. This oversight can lead to unexpected
> >> crashes due to null pointer dereference when incorrect inputs are
> >> provided.
> >>
> >> To address this issue, add required sanity for all input parameters in
> >> the exposed APIs.
> >
> > Please don't be overprotective. Inserting guarding conditions is good,
> > inserting useless guarding conditions is bad, it complicates the driver
> > and makes it harder to follow. Please validate return data rather than
> > adding extra checks to the functions.
>
> Sure, I’ll remove the redundant checks.
> Please see below for explanations.
>
> My intention here is to handle erroneous conditions gracefully to avoid system crashes, as crashes can be detrimental.

Please fix the callers first, rather than adding band-aids.

>
> >>
> >> Signed-off-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
> >> ---
> >>  drivers/firmware/qcom/qcom_tzmem.c | 17 ++++++++++++++++-
> >>  1 file changed, 16 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c
> >> index 92b365178235..2f2e1f2fa9fc 100644
> >> --- a/drivers/firmware/qcom/qcom_tzmem.c
> >> +++ b/drivers/firmware/qcom/qcom_tzmem.c
> >> @@ -203,6 +203,9 @@ qcom_tzmem_pool_new(const struct qcom_tzmem_pool_config *config)
> >>
> >>      might_sleep();
> >>
> >> +    if (!config || !config->policy)
> >
> > config can not be NULL
> > Ack for config->policy check.
>
> Considering a scenario where user doesn't fill config struct details and call devm_qcom_tzmem_pool_new.
> config will be null in that case.

Likewise other driver (not the user!) can pass NULL to other
functions, crashing the kernel. This is not a way to go.

>
> >
> >> +            return ERR_PTR(-EINVAL);
> >> +
> >>      switch (config->policy) {
> >>      case QCOM_TZMEM_POLICY_STATIC:
> >>              if (!config->initial_size)
> >> @@ -316,6 +319,9 @@ devm_qcom_tzmem_pool_new(struct device *dev,
> >>      struct qcom_tzmem_pool *pool;
> >>      int ret;
> >>
> >> +    if (!dev || !config)
> >> +            return ERR_PTR(-EINVAL);
> >
> > dev can not be NULL
> > config can not be NULL
>
> dev may not be always __scm->dev.
> For ex: qcom_qseecom_uefisecapp.c pass it's own dev.
> If new calling driver pass dev as null, will lead to NPD.

Just don't. I don't see other devm_ functions checking the dev param,
because generally we believe in the sanity of driver authors.

>
> >
> >> +
> >>      pool = qcom_tzmem_pool_new(config);
> >>      if (IS_ERR(pool))
> >>              return pool;
> >> @@ -366,7 +372,7 @@ void *qcom_tzmem_alloc(struct qcom_tzmem_pool *pool, size_t size, gfp_t gfp)
> >>      unsigned long vaddr;
> >>      int ret;
> >>
> >> -    if (!size)
> >> +    if (!pool || !size)
> >
> > Is it really possible to pass NULL as pool? Which code path leads to
> > this event?
>
> qcom_tzmem_alloc/free need to be used once pool is already created with devm_qcom_tzmem_pool_new API.
> If pool isn't created, then calling qcom_tzmem_alloc/free will be erroneus.

If your driver doesn't check pool_new() result, then it's broken.

>
> >
> >>              return NULL;
> >>
> >>      size = PAGE_ALIGN(size);
> >> @@ -412,6 +418,9 @@ void qcom_tzmem_free(void *vaddr)
> >>  {
> >>      struct qcom_tzmem_chunk *chunk;
> >>
> >> +    if (!vaddr)
> >> +            return;
> >
> > Ack, simplifies error handling and matches existing kfree-like functions.
> >
> >> +
> >>      scoped_guard(spinlock_irqsave, &qcom_tzmem_chunks_lock)
> >>              chunk = radix_tree_delete_item(&qcom_tzmem_chunks,
> >>                                             (unsigned long)vaddr, NULL);
> >> @@ -446,6 +455,9 @@ phys_addr_t qcom_tzmem_to_phys(void *vaddr)
> >>      void __rcu **slot;
> >>      phys_addr_t ret;
> >>
> >> +    if (!vaddr)
> >
> > Is it possible?
>
> Yes, A scenario where qcom_tzmem_alloc fails resulting vaddr as 0 followed by no null check.
> Now, immediately passing vaddr to qcom_tzmem_to_phys will again cause NPD.

Likewise. If you driver doesn't check qcom_tzmem_alloc(), it's broken
and must be fixed. Null pointer exception will help fix the driver.
Adding such band-aids will hide the issue.

>
> >
> >> +            return 0;
> >> +
> >>      guard(spinlock_irqsave)(&qcom_tzmem_chunks_lock);
> >>
> >>      radix_tree_for_each_slot(slot, &qcom_tzmem_chunks, &iter, 0) {
> >> @@ -466,6 +478,9 @@ EXPORT_SYMBOL_GPL(qcom_tzmem_to_phys);
> >>
> >>  int qcom_tzmem_enable(struct device *dev)
> >>  {
> >> +    if (!dev)
> >> +            return -EINVAL;
> >
> > Definitely not possible.
>
> Ack, by this time __scm->dev will be initialised in qcom_scm driver and cannot be null.
> If some other caller even try and qcom_tzmem_dev is already set hence, return -EBUSY.
> Will drop the check.


-- 
With best wishes
Dmitry

Re: [PATCH 1/2] firmware: qcom: scm: Return -EOPNOTSUPP for unsupported SHM bridge enabling

[Kuldeep Singh]

On 10/7/2024 7:10 AM, Bjorn Andersson wrote:
> On Sat, Oct 05, 2024 at 07:31:49PM GMT, Kuldeep Singh wrote:
> 
> Please shorten the subject a bit, perhaps:
> "firmware: qcom: scm: Improve unsupported SHM bridge detection"
> 
>> From: Qingqing Zhou <quic_qqzhou@quicinc.com>
>>
>> Currently for enabling shm bridge, QTEE will return 0 and put error 4 into
> 
> s/for/when/

Ack.

> 
>> result[0] to qcom_scm for unsupported platform, tzmem will consider this
>> as an unknown error not the unsupported case on the platform.
>>
>> Error log:
>> [    0.177224] qcom_scm firmware:scm: error (____ptrval____): Failed to enable the TrustZone memory allocator
>> [    0.177244] qcom_scm firmware:scm: probe with driver qcom_scm failed with error 4
>>
>> Change the function call qcom_scm_shm_bridge_enable() to remap this
>> result[0] into the unsupported error and then tzmem can consider this as
>> unsupported case instead of reporting an error.
>>
> 
> Sounds like we want a Fixes tag here.

Ack.

> 
>> Signed-off-by: Qingqing Zhou <quic_qqzhou@quicinc.com>
>> Co-developed-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
>> Signed-off-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
>> ---
>>  drivers/firmware/qcom/qcom_scm.c | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index 10986cb11ec0..620313359042 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -111,6 +111,10 @@ enum qcom_scm_qseecom_tz_cmd_info {
>>  	QSEECOM_TZ_CMD_INFO_VERSION		= 3,
>>  };
>>  
>> +enum qcom_scm_shm_bridge_result {
>> +	SHMBRIDGE_RESULT_NOTSUPP	= 4,
>> +};
> 
> This is not an enumeration, but a fixed defined constant. Please use
> #define.

Ack.

>> +
>>  #define QSEECOM_MAX_APP_NAME_SIZE		64
>>  
>>  /* Each bit configures cold/warm boot address for one of the 4 CPUs */
>> @@ -1361,6 +1365,8 @@ EXPORT_SYMBOL_GPL(qcom_scm_lmh_dcvsh_available);
>>  
>>  int qcom_scm_shm_bridge_enable(void)
>>  {
>> +	int ret;
>> +
>>  	struct qcom_scm_desc desc = {
>>  		.svc = QCOM_SCM_SVC_MP,
>>  		.cmd = QCOM_SCM_MP_SHM_BRIDGE_ENABLE,
>> @@ -1373,7 +1379,11 @@ int qcom_scm_shm_bridge_enable(void)
>>  					  QCOM_SCM_MP_SHM_BRIDGE_ENABLE))
>>  		return -EOPNOTSUPP;
>>  
>> -	return qcom_scm_call(__scm->dev, &desc, &res) ?: res.result[0];
>> +	ret = qcom_scm_call(__scm->dev, &desc, &res);
>> +	if (!ret && res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
>> +		return -EOPNOTSUPP;
>> +
>> +	return ret ?: res.result[0];
> 
> I'd prefer, with the additional check, that you'd structure it like this:
> 
> 	if (ret)
> 		return ret;
> 
> 	if (res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
> 		return -EOPNOTSUPP;
> 
> 	return res.result[0];

Sure, above looks more cleaner. Will update in next rev.

> 
> That way we deal with SCM-call errors first, otherwise we inspect and
> act on the returned data.
> 
> That said, the return value of this function, if non-zero, will trickle
> back to and be returned from qcom_scm_probe(), where Linux expects to
> see a valid error code. Are there any other result[0] values we should
> handle, which would allow us to end this function with "return 0"?

As qcom_scm_shm_bridge_enable() is an shm enablement call, need to handle
supported(or unsupported) scenario appropriately and other errors can be
propagated to qcom_tzmem/qcom_scm_probe.

Please note, other return values(related to access control) from QTEE are
failures and do not require conversion to Linux error codes.

-- 
Regards
Kuldeep

Re: [PATCH 2/2] firmware: qcom: qcom_tzmem: Implement sanity checks

[Kuldeep Singh]

On 10/8/2024 1:43 AM, Dmitry Baryshkov wrote:
> On Mon, 7 Oct 2024 at 21:17, Kuldeep Singh <quic_kuldsing@quicinc.com> wrote:
>>
>>
>> On 10/7/2024 1:00 AM, Dmitry Baryshkov wrote:
>>> On Sat, Oct 05, 2024 at 07:31:50PM GMT, Kuldeep Singh wrote:
>>>> The qcom_tzmem driver currently has multiple exposed APIs that lack
>>>> validations on input parameters. This oversight can lead to unexpected
>>>> crashes due to null pointer dereference when incorrect inputs are
>>>> provided.
>>>>
>>>> To address this issue, add required sanity for all input parameters in
>>>> the exposed APIs.
>>>
>>> Please don't be overprotective. Inserting guarding conditions is good,
>>> inserting useless guarding conditions is bad, it complicates the driver
>>> and makes it harder to follow. Please validate return data rather than
>>> adding extra checks to the functions.
>>
>> Sure, I’ll remove the redundant checks.
>> Please see below for explanations.
>>
>> My intention here is to handle erroneous conditions gracefully to avoid system crashes, as crashes can be detrimental.
> 
> Please fix the callers first, rather than adding band-aids.

I see your point and understand the emphasis.
I'll submit v2 as per suggestion.


-- 
Regards
Kuldeep

Re: [PATCH 2/2] firmware: qcom: qcom_tzmem: Implement sanity checks

[Bartosz Golaszewski]

On Mon, 7 Oct 2024 at 22:44, Kuldeep Singh <quic_kuldsing@quicinc.com> wrote:
>
>
>
> On 10/8/2024 1:43 AM, Dmitry Baryshkov wrote:
> > On Mon, 7 Oct 2024 at 21:17, Kuldeep Singh <quic_kuldsing@quicinc.com> wrote:
> >>
> >>
> >> On 10/7/2024 1:00 AM, Dmitry Baryshkov wrote:
> >>> On Sat, Oct 05, 2024 at 07:31:50PM GMT, Kuldeep Singh wrote:
> >>>> The qcom_tzmem driver currently has multiple exposed APIs that lack
> >>>> validations on input parameters. This oversight can lead to unexpected
> >>>> crashes due to null pointer dereference when incorrect inputs are
> >>>> provided.
> >>>>
> >>>> To address this issue, add required sanity for all input parameters in
> >>>> the exposed APIs.
> >>>
> >>> Please don't be overprotective. Inserting guarding conditions is good,
> >>> inserting useless guarding conditions is bad, it complicates the driver
> >>> and makes it harder to follow. Please validate return data rather than
> >>> adding extra checks to the functions.
> >>
> >> Sure, I’ll remove the redundant checks.
> >> Please see below for explanations.
> >>
> >> My intention here is to handle erroneous conditions gracefully to avoid system crashes, as crashes can be detrimental.
> >
> > Please fix the callers first, rather than adding band-aids.
>
> I see your point and understand the emphasis.
> I'll submit v2 as per suggestion.
>

Just to add to what Dmitry said: when you see this kind of checks in
the kernel, it's typically because it makes functional sense for the
API. For instance clk_get_clock_optional() can return NULL and it's
considered a no-error situation but in this case clk_set_rate() must
check whether struct clk * is NULL and it returns 0 as if the
underlying set-rate operation succeeded.

On the other hand there's no such situation where a NULL-pointer
returned by kmalloc() could be considered successful and so we don't
do NULL-checks whenever kmalloc'ed memory is expected as argument.

Similarly here: there's no chance qcom_tzmem_pool_new() will return
NULL so there's no reason to check it and if it returns an ERR_PTR()
then we have to trust the user to check the return value and not pass
it on.

If anything: you could add __must_check to the relevant definitions here.

Bart

Re: [PATCH 1/2] firmware: qcom: scm: Return -EOPNOTSUPP for unsupported SHM bridge enabling

[Bjorn Andersson]

On Tue, Oct 08, 2024 at 02:10:02AM GMT, Kuldeep Singh wrote:
> On 10/7/2024 7:10 AM, Bjorn Andersson wrote:
> > On Sat, Oct 05, 2024 at 07:31:49PM GMT, Kuldeep Singh wrote:
[..]
> >> +
> >>  #define QSEECOM_MAX_APP_NAME_SIZE		64
> >>  
> >>  /* Each bit configures cold/warm boot address for one of the 4 CPUs */
> >> @@ -1361,6 +1365,8 @@ EXPORT_SYMBOL_GPL(qcom_scm_lmh_dcvsh_available);
> >>  
> >>  int qcom_scm_shm_bridge_enable(void)
> >>  {
> >> +	int ret;
> >> +
> >>  	struct qcom_scm_desc desc = {
> >>  		.svc = QCOM_SCM_SVC_MP,
> >>  		.cmd = QCOM_SCM_MP_SHM_BRIDGE_ENABLE,
> >> @@ -1373,7 +1379,11 @@ int qcom_scm_shm_bridge_enable(void)
> >>  					  QCOM_SCM_MP_SHM_BRIDGE_ENABLE))
> >>  		return -EOPNOTSUPP;
> >>  
> >> -	return qcom_scm_call(__scm->dev, &desc, &res) ?: res.result[0];
> >> +	ret = qcom_scm_call(__scm->dev, &desc, &res);
> >> +	if (!ret && res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
> >> +		return -EOPNOTSUPP;
> >> +
> >> +	return ret ?: res.result[0];
> > 
> > I'd prefer, with the additional check, that you'd structure it like this:
> > 
> > 	if (ret)
> > 		return ret;
> > 
> > 	if (res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
> > 		return -EOPNOTSUPP;
> > 
> > 	return res.result[0];
> 
> Sure, above looks more cleaner. Will update in next rev.
> 

Thanks!

> > 
> > That way we deal with SCM-call errors first, otherwise we inspect and
> > act on the returned data.
> > 
> > That said, the return value of this function, if non-zero, will trickle
> > back to and be returned from qcom_scm_probe(), where Linux expects to
> > see a valid error code. Are there any other result[0] values we should
> > handle, which would allow us to end this function with "return 0"?
> 
> As qcom_scm_shm_bridge_enable() is an shm enablement call, need to handle
> supported(or unsupported) scenario appropriately and other errors can be
> propagated to qcom_tzmem/qcom_scm_probe.
> 
> Please note, other return values(related to access control) from QTEE are
> failures and do not require conversion to Linux error codes.
> 

I'm not familiar with the value space of such errors, but any value
other than -EOPNOTSUPP and 0 returned here will propagate back and be
the value returned to the driver core.

It seems reasonable to ensure that the return value space makes sense to
Linux, just in case something up the stack decides to act upon the value
we return.

Regards,
Bjorn