Re: [PATCH 1/2] soc: qcom: smem: add boundary checks for partitions

[Albert Esteve]

On Tue, Sep 1, 2026 at 12:54 AM Bjorn Andersson <andersson@xxxxxxxxxx> wrote:
>
> On Thu, Aug 20, 2026 at 01:34:18PM +0200, Albert Esteve wrote:
> > From: Sudeepgoud Patil <quic_sudeepgo@xxxxxxxxxxx>
> >
> > Add condition check to make sure that the end address
> > of private entry does not go out of partition.
> >
> > Signed-off-by: Sarannya S <quic_sarannya@xxxxxxxxxxx>
> > Signed-off-by: Pranav Mahesh Phansalkar <quic_pphansal@xxxxxxxxxxx>
> > Signed-off-by: Sudeepgoud Patil <quic_sudeepgo@xxxxxxxxxxx>
> > Signed-off-by: Albert Esteve <aesteve@xxxxxxxxxx>
>
> I'm sorry, Albert. Sudeepgoud messed up the authorship vs signed-off-by
> chain here. The author should be "Sarannya", as that's the first
> signed-off-by.
>
> PS. It would also be nice if this was submitted with updated email
> addresses and copyright statement. But I'd not expect you to fix that.

Hi Bjorn,

I'm happy to address these changes for V2. Do you know where to find
the updated email addresses?

BR,
Albert.

>
> Regards,
> Bjorn
>
> > ---
> >  drivers/soc/qcom/smem.c | 105 +++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 72 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> > index afb21a778fe7b..194ffb2ac010f 100644
> > --- a/drivers/soc/qcom/smem.c
> > +++ b/drivers/soc/qcom/smem.c
> > @@ -2,6 +2,7 @@
> >  /*
> >   * Copyright (c) 2015, Sony Mobile Communications AB.
> >   * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
> > + * Copyright (c) 2023-2024 Qualcomm Innovation Center, Inc. All rights reserved.
> >   */
> >
> >  #include <linux/hwspinlock.h>
> > @@ -85,6 +86,17 @@
> >  /* Processor/host identifier for the global partition */
> >  #define SMEM_GLOBAL_HOST     0xfffe
> >
> > +/* Entry range check
> > + * ptr >= start : Checks if ptr is greater than the start of access region
> > + * ptr + size >= ptr: Check for integer overflow (On 32bit system where ptr
> > + * and size are 32bits, ptr + size can wrap around to be a small integer)
> > + * ptr + size <= end: Checks if ptr+size is less than the end of access region
> > + */
> > +#define IN_PARTITION_RANGE(ptr, size, start, end)            \
> > +     (((void *)(ptr) >= (void *)(start)) &&                  \
> > +      (((void *)(ptr) + (size)) >= (void *)(ptr)) && \
> > +      (((void *)(ptr) + (size)) <= (void *)(end)))
> > +
> >  /**
> >    * struct smem_proc_comm - proc_comm communication struct (legacy)
> >    * @command:        current command to be executed
> > @@ -403,6 +415,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;
> > @@ -415,19 +428,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))
> >               return -EINVAL;
> >
> > -     while (hdr < end) {
> > +     while ((hdr < end) && ((hdr + 1) < end)) {
> >               if (hdr->canary != SMEM_PRIVATE_CANARY)
> >                       goto bad_canary;
> >               if (le16_to_cpu(hdr->item) == item)
> >                       return -EEXIST;
> >
> > -             hdr = uncached_entry_next(hdr);
> > +             next_hdr = uncached_entry_next(hdr);
> > +
> > +             if (WARN_ON(next_hdr <= hdr))
> > +                     return -EINVAL;
> > +
> > +             hdr = next_hdr;
> >       }
> >
> > -     if (WARN_ON((void *)hdr > p_end))
> > +     if (WARN_ON((void *)hdr > (void *)end))
> >               return -EINVAL;
> >
> >       /* Check that we don't grow into the cached region */
> > @@ -587,9 +606,11 @@ static void *qcom_smem_get_private(struct qcom_smem *smem,
> >                                  unsigned item,
> >                                  size_t *size)
> >  {
> > -     struct smem_private_entry *e, *end;
> > +     struct smem_private_entry *e, *uncached_end, *cached_end;
> > +     struct smem_private_entry *next_e;
> >       struct smem_partition_header *phdr;
> >       void *item_ptr, *p_end;
> > +     size_t entry_size = 0;
> >       u32 padding_data;
> >       u32 e_size;
> >
> > @@ -597,67 +618,85 @@ static void *qcom_smem_get_private(struct qcom_smem *smem,
> >       p_end = (void *)phdr + part->size;
> >
> >       e = phdr_to_first_uncached_entry(phdr);
> > -     end = phdr_to_last_uncached_entry(phdr);
> > +     uncached_end = phdr_to_last_uncached_entry(phdr);
> > +     cached_end = phdr_to_last_cached_entry(phdr);
> > +
> > +     if (WARN_ON(!IN_PARTITION_RANGE(uncached_end, 0, phdr, cached_end)
> > +                                     || (void *)cached_end > p_end))
> > +             return ERR_PTR(-EINVAL);
> >
> > -     while (e < end) {
> > +     while ((e < uncached_end) && ((e + 1) < uncached_end)) {
> >               if (e->canary != SMEM_PRIVATE_CANARY)
> >                       goto invalid_canary;
> >
> >               if (le16_to_cpu(e->item) == item) {
> > -                     if (size != NULL) {
> > -                             e_size = le32_to_cpu(e->size);
> > -                             padding_data = le16_to_cpu(e->padding_data);
> > +                     e_size = le32_to_cpu(e->size);
> > +                     padding_data = le16_to_cpu(e->padding_data);
> >
> > -                             if (WARN_ON(e_size > part->size || padding_data > e_size))
> > -                                     return ERR_PTR(-EINVAL);
> > +                     if (e_size < part->size && padding_data < e_size)
> > +                             entry_size = e_size - padding_data;
> > +                     else
> > +                             return ERR_PTR(-EINVAL);
> >
> > -                             *size = e_size - padding_data;
> > -                     }
> > +                     item_ptr =  uncached_entry_to_item(e);
> >
> > -                     item_ptr = uncached_entry_to_item(e);
> > -                     if (WARN_ON(item_ptr > p_end))
> > +                     if (WARN_ON(!IN_PARTITION_RANGE(item_ptr, entry_size, e, uncached_end)))
> >                               return ERR_PTR(-EINVAL);
> >
> > +                     if (size != NULL)
> > +                             *size = entry_size;
> > +
> >                       return item_ptr;
> >               }
> >
> > -             e = uncached_entry_next(e);
> > -     }
> > +             next_e = uncached_entry_next(e);
> > +             if (WARN_ON(next_e <= e))
> > +                     return ERR_PTR(-EINVAL);
> >
> > -     if (WARN_ON((void *)e > p_end))
> > +             e = next_e;
> > +     }
> > +     if (WARN_ON((void *)e > (void *)uncached_end))
> >               return ERR_PTR(-EINVAL);
> >
> >       /* Item was not found in the uncached list, search the cached list */
> >
> > +     if (cached_end == p_end)
> > +             return ERR_PTR(-ENOENT);
> > +
> >       e = phdr_to_first_cached_entry(phdr, part->cacheline);
> > -     end = phdr_to_last_cached_entry(phdr);
> >
> > -     if (WARN_ON((void *)e < (void *)phdr || (void *)end > p_end))
> > +     if (WARN_ON(!IN_PARTITION_RANGE(cached_end, 0, uncached_end, p_end) ||
> > +                     !IN_PARTITION_RANGE(e, sizeof(*e), cached_end, p_end)))
> >               return ERR_PTR(-EINVAL);
> >
> > -     while (e > end) {
> > +     while (e > cached_end) {
> >               if (e->canary != SMEM_PRIVATE_CANARY)
> >                       goto invalid_canary;
> >
> >               if (le16_to_cpu(e->item) == item) {
> > -                     if (size != NULL) {
> > -                             e_size = le32_to_cpu(e->size);
> > -                             padding_data = le16_to_cpu(e->padding_data);
> > +                     e_size = le32_to_cpu(e->size);
> > +                     padding_data = le16_to_cpu(e->padding_data);
> >
> > -                             if (WARN_ON(e_size > part->size || padding_data > e_size))
> > -                                     return ERR_PTR(-EINVAL);
> > -
> > -                             *size = e_size - padding_data;
> > -                     }
> > +                     if (e_size < part->size && padding_data < e_size)
> > +                             entry_size  = e_size - padding_data;
> > +                     else
> > +                             return ERR_PTR(-EINVAL);
> >
> > -                     item_ptr = cached_entry_to_item(e);
> > -                     if (WARN_ON(item_ptr < (void *)phdr))
> > +                     item_ptr =  cached_entry_to_item(e);
> > +                     if (WARN_ON(!IN_PARTITION_RANGE(item_ptr, entry_size, cached_end, e)))
> >                               return ERR_PTR(-EINVAL);
> >
> > +                     if (size != NULL)
> > +                             *size = entry_size;
> > +
> >                       return item_ptr;
> >               }
> >
> > -             e = cached_entry_next(e, part->cacheline);
> > +             next_e = cached_entry_next(e, part->cacheline);
> > +             if (WARN_ON(next_e >= e))
> > +                     return ERR_PTR(-EINVAL);
> > +
> > +             e = next_e;
> >       }
> >
> >       if (WARN_ON((void *)e < (void *)phdr))
> >
> > --
> > 2.55.0
> >
>