Re: [PATCH 1/2] soc: qcom: smem: introduce qcom_smem_validate_item API

[Kathiravan Thirumoorthy]

On 10/31/2025 6:10 AM, Christopher Lew wrote:
> On Thu, Oct 30, 2025 at 9:48 AM Bjorn Andersson <andersson@xxxxxxxxxx> wrote:
> > On Thu, Oct 30, 2025 at 03:07:48PM +0530, Kathiravan Thirumoorthy wrote:
> > > When a SMEM item is allocated or retrieved, sanity check on the SMEM item
> > > is performed and backtrace is printed if the SMEM item is invalid.
> > That sounds overly defensive...
> Discussed with Bjorn a bit offline and we couldn't come up with a
> really good reason to keep the WARN_ON() check.
> Dropping WARN_ON() and returning an error back from qcom_smem_get()
> that clients can act on should suffice.


Thanks Bjorn and Chris for the comments and the suggestions. Based on 
the below feedback from Chris, I understand that we can just drop the 
WARN_ON() on both places. I will submit the V2 with that address.


> > > Image version table in SMEM contains version details for the first 32
> > > images. Beyond that, another SMEM item 667 is being used, which may not
> > > be defined in all the platforms. So directly retrieving the SMEM item 667,
> > > throws the warning as invalid item number.
> > >
> > > To handle such cases, introduce a new API to validate the SMEM item before
> > > processing it. While at it, make use of this API in the SMEM driver where
> > > possible.
> > >
> > > Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@xxxxxxxxxxxxxxxx>
> > > ---
> > >   drivers/soc/qcom/smem.c       | 16 ++++++++++++++--
> > >   include/linux/soc/qcom/smem.h |  1 +
> > >   2 files changed, 15 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> > > index c4c45f15dca4fb14f97df4ad494c1189e4f098bd..8a0a832f1e9915b2177a0fe08298ffe8a779e516 100644
> > > --- a/drivers/soc/qcom/smem.c
> > > +++ b/drivers/soc/qcom/smem.c
> > > @@ -396,6 +396,18 @@ bool qcom_smem_is_available(void)
> > >   }
> > >   EXPORT_SYMBOL_GPL(qcom_smem_is_available);
> > >
> > > +/**
> > > + * qcom_smem_validate_item() - Check if SMEM item is within the limit
> > If nothing else, this contradicts the comment by SMEM_ITEM_COUNT.
> >
> > > + * @item:    SMEM item to validate
> > > + *
> > > + * Return: true if SMEM item is valid, false otherwise.
> > > + */
> > > +bool qcom_smem_validate_item(unsigned item)
> > > +{
> > > +     return item < __smem->item_count;
> > > +}
> > > +EXPORT_SYMBOL_GPL(qcom_smem_validate_item);
> > > +
> > >   static int qcom_smem_alloc_private(struct qcom_smem *smem,
> > >                                   struct smem_partition *part,
> > >                                   unsigned item,
> > > @@ -517,7 +529,7 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
> > >                return -EINVAL;
> > >        }
> > >
> > > -     if (WARN_ON(item >= __smem->item_count))
> > > +     if (WARN_ON(!qcom_smem_validate_item(item)))
> > When we're using a version 11 (global heap, with toc indexed by the item
> > number) the SMEM_ITEM_COUNT actually matters, but when we use version 12
> > the items are stored in linked lists, so the only limit I can see is
> > that the item needs to be max 16 bit.
> >
> > I think we should push this check down to qcom_smem_alloc_global().
> >
> > And have a sanity check for item in qcom_smem_alloc_private() and
> > qcom_smem_get_private() to avoid truncation errors.
> For alloc, I think we should adhere to the platform defined max
> item_count. I'm not sure if the remote hosts validate the entries
> against item_count while they are iterating through the items during
> their implementation of qcom_smem_get().
>
> > >                return -EINVAL;
> > >
> > >        ret = hwspin_lock_timeout_irqsave(__smem->hwlock,
> > > @@ -690,7 +702,7 @@ void *qcom_smem_get(unsigned host, unsigned item, size_t *size)
> > >        if (!__smem)
> > >                return ptr;
> > >
> > > -     if (WARN_ON(item >= __smem->item_count))
> > > +     if (WARN_ON(!qcom_smem_validate_item(item)))
> > I think we should push this check down to qcom_smem_get_global()
> >
> > I guess we'd still hit your problem on version 11 platforms if we keep
> > the WARN_ON(), but I don't know why that's reason for throwing a splat
> > in the log. Let's drop the WARN_ON() as well.
> I think it's worth keeping the item_count check here because it acts
> as a quick short circuit out of the search loop for absurd values in
> qcom_smem_get_private().
>
> Thanks,
> Chris
>
> > >                return ERR_PTR(-EINVAL);
> > >
> > >        if (host < SMEM_HOST_COUNT && __smem->partitions[host].virt_base) {
> > > diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
> > > index f946e3beca215548ac56dbf779138d05479712f5..05891532d530a25747afb8dc96ad4ba668598197 100644
> > > --- a/include/linux/soc/qcom/smem.h
> > > +++ b/include/linux/soc/qcom/smem.h
> > > @@ -5,6 +5,7 @@
> > >   #define QCOM_SMEM_HOST_ANY -1
> > >
> > >   bool qcom_smem_is_available(void);
> > > +bool qcom_smem_validate_item(unsigned item);
> > This makes the API clunky for no real reason, let's avoid that.
> >
> >
> > Adding Chris, in case I'm overlooking something here.
> >
> > Regards,
> > Bjorn
> >
> > >   int qcom_smem_alloc(unsigned host, unsigned item, size_t size);
> > >   void *qcom_smem_get(unsigned host, unsigned item, size_t *size);
> > >
> > >
> > > --
> > > 2.34.1