Message-ID: <7788e25c-4622-4c7d-86b8-0827aac917af@oss.qualcomm.com>
Date: Fri, 31 Oct 2025 10:05:19 +0530
From: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@....qualcomm.com>
To: Christopher Lew <christopher.lew@....qualcomm.com>,
        Bjorn Andersson <andersson@...nel.org>
Cc: Chris Lew <chris.lew@....qualcomm.com>,
        Konrad Dybcio <konradybcio@...nel.org>, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] soc: qcom: smem: introduce qcom_smem_validate_item
 API


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

Powered by blists - more mailing lists