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.

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@oss.qualcomm.com>
---
 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
+ * @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)))
 		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)))
 		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);
 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

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...

> 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@oss.qualcomm.com>
> ---
>  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.

>  		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.

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

On Thu, Oct 30, 2025 at 9:48 AM Bjorn Andersson <andersson@kernel.org> 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.

> > 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@oss.qualcomm.com>
> > ---
> >  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
> >

On 10/31/2025 6:10 AM, Christopher Lew wrote:
> On Thu, Oct 30, 2025 at 9:48 AM Bjorn Andersson <andersson@kernel.org> 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@oss.qualcomm.com>
>>> ---
>>>   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
>>>