Re: [PATCH v2 1/2] xen/list: avoid UB in list iterators

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 2/20/25 2:38 AM, Stefano Stabellini wrote:

> On Wed, 19 Feb 2025, Andrew Cooper wrote:
> > On 19/02/2025 2:18 pm, Juergen Gross wrote:
> > > The list_for_each_entry*() iterators are testing for having reached the
> > > end of the list in a way which relies on undefined behavior: the
> > > iterator (being a pointer to the struct of a list element) is advanced
> > > and only then tested to have reached not the next element, but the list
> > > head. This results in the list head being addressed via a list element
> > > pointer, which is undefined, in case the list elements have a higher
> > > alignment than the list head.
> > >
> > > Avoid that by testing for the end of the list before advancing the
> > > iterator. In case of having reached the end of the list, set the
> > > iterator to NULL and use that for stopping the loop. This has the
> > > additional advantage of not leaking the iterator pointing to something
> > > which isn't a list element past the loop.
> > >
> > > There is one case in the Xen code where the iterator is used after
> > > the loop: it is tested to be non-NULL to indicate the loop has run
> > > until reaching the end of the list. This case is modified to use the
> > > iterator being NULL for indicating the end of the list has been
> > > reached.
> > >
> > > Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > >         
> > I agree there's an issue here, but as said before, I do not agree with
> > this patch.
> >
> > For starters, bloat-o-meter on a random top-of-tree build says
> >
> >     add/remove: 8/1 grow/shrink: 112/68 up/down: 4314/-2855 (1459)
> >
> > which is a horrible overhead for a case where the sequence of
> > instructions are correct (only the C level types are a problem) and ...
> >
> > > ---
> > > No proper Fixes: tag, as this bug predates Xen's git and mercurial
> > > history.
> > > V2:
> > > - fix one use case (Jan Beulich)
> > > - let list_is_first() return bool, rename parameter (Jan Beulich)
> > > - paranthesize iterator when used as non-NULL test (Jan Beulich)
> > > - avoid dereferencing NULL in the safe iterators (Jan Beulich)
> > > ---
> > >  xen/drivers/passthrough/x86/hvm.c |   3 +-
> > >         
> > ... the need for this adjustment being discovered after-the-fact means
> > it's a very risky change at this juncture in the release.
> >       
> I have not reviewed the patch in enough detail to form an opinion on the
> approach yet. However, I want to note that I also don't think that this
> series should be committed at this stage of the release process. It
> would be better to wait for the 4.21 release cycle.
>     
Based on the comments above lets consider then this patch to be merged to 4.21.

Thanks.

~ Oleksii