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

[Albert Esteve]

On Thu, Oct 22, 2026 at 10:20:05AM +0200, Albert Esteve wrote:
> On Wed, Oct 21, 2026 at 2:39 PM Konrad Dybcio
> <konrad.dybcio@xxxxxxxxxxxxxxxx> wrote:
> >
> > On 9/24/26 3:10 PM, Albert Esteve wrote:
> > > From: Sarannya S <quic_sarannya@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 <quic_sarannya@xxxxxxxxxxxxxxxx>
> > > Signed-off-by: Pranav Mahesh Phansalkar <quic_pphansal@xxxxxxxxxxxxxxxx>
> > > Signed-off-by: Sudeepgoud Patil <quic_sudeepgo@xxxxxxxxxxxxxxxx>
> > > Signed-off-by: Albert Esteve <aesteve@xxxxxxxxxx>
> > > ---
> >
> > [...]
> >
> > > -     while (hdr < end) {
> > > +     while ((hdr + 1) < end) {
> >
> > This will reject a header that's at the tail of the buffer, need
> > to make this <= instead
> 
> Roger. This is a bit hard to read as intuitively I would have thought
> end is the first free byte after the last header, so `(hdr + 1) < end`
> should include it? End uses `offset_free_uncached` which should be
> bigger than `hdr + 1` and it would not miss the tail. But being
> defensive about its value seems like the right call. I will change it.
> 
> >
> > [...]
> >
> > > -     while (e < end) {
> > > +     while ((e + 1) < uncached_end) {
> >
> > Likewise
> >
> > [...]
> >
> > > -             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))
> >
> > And LLM thinks should compare against cached_end
> 
> This is preexisting but I think the LLM is right, and it matches the
> theme of the patch. I will change it.

I've sent a new version and Sashiko still complains. Maybe we can just drop the check?

We can continue the conversation in the new version of the series. I wrote here because it had the context.

BR,
Albert

> 
> >
> > Konrad
> >
>