Re: [PATCH v4 2/4] soc: qcom: smem: validate private entry list walks

[Albert Esteve]

On Tue, Oct 27, 2026 at 01:45:26PM +0100, Konrad Dybcio wrote:
> On 10/22/26 11:47 AM, Albert Esteve wrote:
> > From: Sarannya S <sarannya.s@xxxxxxxxxxxxxxxx>
> > 
> > When walking private entries, use the uncached/cached free offsets as
> > the list bounds, require room for a header before dereferencing, and
> > reject a next pointer that does not advance.
> > 
> > Skip the cached list when the cached free offset is at the partition
> > end.
> > 
> > Signed-off-by: Sarannya S <sarannya.s@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Pranav Mahesh Phansalkar <pranav.phansalkar@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Sudeepgoud Patil <sudeep.patil@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Albert Esteve <aesteve@xxxxxxxxxx>
> > ---
> >  drivers/soc/qcom/smem.c | 52 ++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 38 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> > index 36a4a10f3aec..a6732832e2c5 100644
> > --- a/drivers/soc/qcom/smem.c
> > +++ b/drivers/soc/qcom/smem.c
> > @@ -422,6 +422,7 @@ static int qcom_smem_alloc_private(struct qcom_smem *smem,
> >  				   size_t size)
> >  {
> >  	struct smem_private_entry *hdr, *end;
> > +	struct smem_private_entry *next_hdr;
> >  	struct smem_partition_header *phdr;
> >  	size_t alloc_size;
> >  	void *cached;
> > @@ -434,19 +435,25 @@ static int qcom_smem_alloc_private(struct qcom_smem *smem,
> >  	end = phdr_to_last_uncached_entry(phdr);
> >  	cached = phdr_to_last_cached_entry(phdr);
> >  
> > -	if (WARN_ON((void *)end > p_end || cached > p_end))
> > +	if (WARN_ON(!IN_PARTITION_RANGE(end, 0, phdr, cached) ||
> > +			cached > p_end))
> 
> nit: i wouldn't break this line, and otherwise the next line should
> start below where the first condition starts

Right. The statement fits in one line so will do that.

> 
> [...]
> 
> > -	if (WARN_ON((void *)hdr > p_end))
> > +	if (WARN_ON((void *)hdr > (void *)end))
> 
> LLM noticed that before this patch, we risk overrunning end, but with
> this patch, we now risk a corrupted/malicious partition ending before
> 'end'. A valid walk always lands exactly on `end`, so the fix is:
> 
> if (WARN_ON(hdr != end))
>     return -EINVAL;

I see, makes sense. Will do as suggested.

> 
> 
> Otherwise my clanker is reasonably happy and I see nothing wrong
> 
> Reviewed-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>

Thanks!

> 
> Konrad