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@xxxxxxxxxxx>
> ---
> 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
>
