Hi all,
While reading drivers/video/fbdev/core/fb_defio.c I noticed
something that looks like a past the end iterator pattern.
I would appreciate it if you could take a look and let me
know whether this is a real issue, and whether it is worth
fixing.
The site is fb_deferred_io_pageref_get() (linux-7.1-rc1,
around line 203):
list_for_each_entry(cur, &fbdefio_state->pagereflist, list) {
if (cur->offset > pageref->offset)
break;
}
pos = &cur->list;
When the loop walks all entries without break, cur has gone
one step past the last entry. &cur->list then aliases
&fbdefio_state->pagereflist (the list head) via container_of
offset cancellation, so pos equals the head and the
subsequent list_add_tail(&pageref->list, pos) lands at the
list tail. That is the intended behaviour, but the access
is undefined per C11.
Jakob Koschel cleaned up many such sites in 2022, for example
commits 99d8ae4ec8a (tracing: Remove usage of list iterator
variable after the loop), 2966a9918df (clockevents: Use dedicated
list iterator variable) and dc1acd5c946 (dlm: replace usage of
found with dedicated list iterator variable). This site in
fb_defio was not covered.
A candidate fix would move the pos = &cur->list assignment
inside the break arm, so pos is only overwritten when the
loop actually finds a position. The default value of pos
(set earlier in the function to &fbdefio_state->pagereflist)
already covers the loop fall through case. The observable
behaviour is unchanged.
If this is intentional or already known, please disregard.
Otherwise, I am happy to send a [PATCH] or to leave the fix
to you. Thank you for your time, and sorry for the noise if
this is not actually worth fixing or has already been spotted.
Thanks,
Maoyi Xie
https://maoyixie.com/
