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