linux-kernel - Re: [PATCH 1/2] soc: qcom: smem: introduce qcom_smem_validate

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b46wt76zmlms5h6zkner2rr22hwmsz422jy44qziqe6a2c4qrt@i5x7j6vgrzqo>
Date: Thu, 30 Oct 2025 11:51:50 -0500
From: Bjorn Andersson <andersson@...nel.org>
To: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@....qualcomm.com>, 
	Chris Lew <chris.lew@....qualcomm.com>
Cc: 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 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@....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
>