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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew