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

[Albert Esteve]

On Wed, Sep 23, 2026 at 3:58 PM Konrad Dybcio
<konrad.dybcio@xxxxxxxxxxxxxxxx> wrote:
>
> On 9/4/26 5:18 PM, Albert Esteve wrote:
> > From: Sarannya S <quic_sarannya@xxxxxxxxxxxxxxxx>
> >
> > Add condition check to make sure that the end address
> > of private entry does not go out of partition.
>
> [...]
>
> > @@ -409,6 +421,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;
> > @@ -421,19 +434,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))
>
> strange indentation
> >               return -EINVAL;
> >
> > -     while (hdr < end) {
> > +     while ((hdr < end) && ((hdr + 1) < end)) {
>
> I believe the latter implies the former

Yes, the check that matter is the second part. I'll fix it in the next
version, thanks!

>
> [...]
>
> >       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))
>
> The || usually goes on the end of the line, please move it there
>
> This patch changes a lot without much explanation, could you please
> split it up so that the changes are more focused? I know the general
> theme is overflow checks, but a lot of conditions change
> simultaneously in its current form and it's hard to track, even GPT
> is slightly confused

Yeah, you're right. I was preserving the original patch as is, but I
will try to split it in a way that makes sense, while keeping the
original SoBs.

BR,
Albert

>
> Konrad
>