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.coop...@citrix.com> > Signed-off-by: Juergen Gross <jgr...@suse.com> > Release-Acked-by: Oleksii Kurochko <oleksii.kuroc...@gmail.com>
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
