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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.