diff mbox series

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

Message ID 20250219141818.8789-2-jgross@suse.com (mailing list archive)
State New
Headers show
Series xen/list: some fixes in list.h | expand

Commit Message

Juergen Gross Feb. 19, 2025, 2:18 p.m. UTC
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@citrix.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
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 +-
 xen/include/xen/list.h            | 110 +++++++++++++++++++-----------
 2 files changed, 73 insertions(+), 40 deletions(-)

Comments

Andrew Cooper Feb. 19, 2025, 3:17 p.m. UTC | #1
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@citrix.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@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
Stefano Stabellini Feb. 20, 2025, 1:38 a.m. UTC | #2
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@citrix.com>
> > Signed-off-by: Juergen Gross <jgross@suse.com>
> > Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@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.

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.
Oleksii Kurochko Feb. 20, 2025, 10:44 a.m. UTC | #3
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@citrix.com>
>>> Signed-off-by: Juergen Gross<jgross@suse.com>
>>> Release-Acked-by: Oleksii Kurochko<oleksii.kurochko@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.
> 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
diff mbox series

Patch

diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c
index f5faff7a49..213dd60340 100644
--- a/xen/drivers/passthrough/x86/hvm.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -639,12 +639,11 @@  int pt_irq_destroy_bind(
             {
                 list_del(&girq->list);
                 xfree(girq);
-                girq = NULL;
                 break;
             }
         }
 
-        if ( girq )
+        if ( !girq )
         {
             write_unlock(&d->event_lock);
             return -EINVAL;
diff --git a/xen/include/xen/list.h b/xen/include/xen/list.h
index 62169f4674..d90b3f6f0d 100644
--- a/xen/include/xen/list.h
+++ b/xen/include/xen/list.h
@@ -291,6 +291,17 @@  static inline void list_move_tail(struct list_head *list,
     list_add_tail(list, head);
 }
 
+/**
+ * list_is_first - tests whether @entry is the first entry in list @head
+ * @entry: the entry to test
+ * @head: the head of the list
+ */
+static inline bool list_is_first(const struct list_head *entry,
+                                 const struct list_head *head)
+{
+    return entry->prev == head;
+}
+
 /**
  * list_is_last - tests whether @list is the last entry in list @head
  * @list: the entry to test
@@ -440,7 +451,19 @@  static inline void list_splice_init(struct list_head *list,
   */
 #define list_next_entry(pos, member) \
         list_entry((pos)->member.next, typeof(*(pos)), member)
- 
+
+/**
+  * list_next_entry_or_null - get the next element in list
+  * @pos:        the type * to cursor
+  * @member:     the name of the struct list_head within the struct.
+  *
+  * Note that if the end of the list is reached, it returns NULL.
+  */
+#define list_next_entry_or_null(head, pos, member)                 \
+        ((!(pos) || list_is_last(&(pos)->member, head))            \
+         ? NULL                                                    \
+         : list_entry((pos)->member.next, typeof(*(pos)), member))
+
 /**
   * list_prev_entry - get the prev element in list
   * @pos:        the type * to cursor
@@ -449,6 +472,18 @@  static inline void list_splice_init(struct list_head *list,
 #define list_prev_entry(pos, member) \
         list_entry((pos)->member.prev, typeof(*(pos)), member)
 
+/**
+  * list_prev_entry_or_null - get the prev element in list
+  * @pos:        the type * to cursor
+  * @member:     the name of the struct list_head within the struct.
+  *
+  * Note that if the start of the list is reached, it returns NULL.
+  */
+#define list_prev_entry_or_null(head, pos, member)                 \
+        ((!(pos) || list_is_first(&(pos)->member, head))           \
+         ? NULL                                                    \
+         : list_entry((pos)->member.prev, typeof(*(pos)), member))
+
 /**
  * list_for_each    -    iterate over a list
  * @pos:    the &struct list_head to use as a loop cursor.
@@ -492,10 +527,10 @@  static inline void list_splice_init(struct list_head *list,
  * @head:   the head for your list.
  * @member: the name of the list_struct within the struct.
  */
-#define list_for_each_entry(pos, head, member)                          \
-    for ((pos) = list_entry((head)->next, typeof(*(pos)), member);      \
-         &(pos)->member != (head);                                      \
-         (pos) = list_entry((pos)->member.next, typeof(*(pos)), member))
+#define list_for_each_entry(pos, head, member)                            \
+    for ( (pos) = list_first_entry_or_null(head, typeof(*(pos)), member); \
+          (pos);                                                          \
+          (pos) = list_next_entry_or_null(head, pos, member) )
 
 /**
  * list_for_each_entry_reverse - iterate backwards over list of given type.
@@ -503,10 +538,10 @@  static inline void list_splice_init(struct list_head *list,
  * @head:   the head for your list.
  * @member: the name of the list_struct within the struct.
  */
-#define list_for_each_entry_reverse(pos, head, member)                  \
-    for ((pos) = list_entry((head)->prev, typeof(*(pos)), member);      \
-         &(pos)->member != (head);                                      \
-         (pos) = list_entry((pos)->member.prev, typeof(*(pos)), member))
+#define list_for_each_entry_reverse(pos, head, member)                   \
+    for ( (pos) = list_last_entry_or_null(head, typeof(*(pos)), member); \
+          (pos);                                                         \
+          (pos) = list_prev_entry_or_null(head, pos, member) )
 
 /**
  * list_prepare_entry - prepare a pos entry for use in
@@ -530,10 +565,10 @@  static inline void list_splice_init(struct list_head *list,
  * Continue to iterate over list of given type, continuing after
  * the current position.
  */
-#define list_for_each_entry_continue(pos, head, member)                   \
-    for ((pos) = list_entry((pos)->member.next, typeof(*(pos)), member);  \
-         &(pos)->member != (head);                                        \
-         (pos) = list_entry((pos)->member.next, typeof(*(pos)), member))
+#define list_for_each_entry_continue(pos, head, member)        \
+    for ( (pos) = list_next_entry_or_null(head, pos, member);  \
+          (pos);                                               \
+          (pos) = list_next_entry_or_null(head, pos, member) )
 
 /**
  * list_for_each_entry_from - iterate over list of given type from the
@@ -544,9 +579,8 @@  static inline void list_splice_init(struct list_head *list,
  *
  * Iterate over list of given type, continuing from current position.
  */
-#define list_for_each_entry_from(pos, head, member)                     \
-    for (; &(pos)->member != (head);                                    \
-         (pos) = list_entry((pos)->member.next, typeof(*(pos)), member))
+#define list_for_each_entry_from(pos, head, member)            \
+    for ( ; (pos); (pos) = list_next_entry_or_null(head, pos, member) )
 
 /**
  * list_for_each_entry_safe - iterate over list of given type safe
@@ -556,11 +590,11 @@  static inline void list_splice_init(struct list_head *list,
  * @head:   the head for your list.
  * @member: the name of the list_struct within the struct.
  */
-#define list_for_each_entry_safe(pos, n, head, member)                  \
-    for ((pos) = list_entry((head)->next, typeof(*(pos)), member),      \
-         (n) = list_entry((pos)->member.next, typeof(*(pos)), member);  \
-         &(pos)->member != (head);                                      \
-         (pos) = (n), (n) = list_entry((n)->member.next, typeof(*(n)), member))
+#define list_for_each_entry_safe(pos, n, head, member)                     \
+    for ( (pos) = list_first_entry_or_null(head, typeof(*(pos)), member),  \
+          (n) = (pos) ? list_next_entry_or_null(head, pos, member) : NULL; \
+          (pos);                                                           \
+          (pos) = (n), (n) = list_next_entry_or_null(head, n, member) )
 
 /**
  * list_for_each_entry_safe_continue
@@ -572,11 +606,11 @@  static inline void list_splice_init(struct list_head *list,
  * Iterate over list of given type, continuing after current point,
  * safe against removal of list entry.
  */
-#define list_for_each_entry_safe_continue(pos, n, head, member)           \
-    for ((pos) = list_entry((pos)->member.next, typeof(*(pos)), member),  \
-         (n) = list_entry((pos)->member.next, typeof(*(pos)), member);    \
-         &(pos)->member != (head);                                        \
-         (pos) = (n), (n) = list_entry((n)->member.next, typeof(*(n)), member))
+#define list_for_each_entry_safe_continue(pos, n, head, member)            \
+    for ( (pos) = list_next_entry_or_null(head, pos, member),              \
+          (n) = (pos) ? list_next_entry_or_null(head, pos, member) : NULL; \
+          (pos);                                                           \
+          (pos) = (n), (n) = list_next_entry_or_null(head, n, member) )
 
 /**
  * list_for_each_entry_safe_from
@@ -589,9 +623,9 @@  static inline void list_splice_init(struct list_head *list,
  * removal of list entry.
  */
 #define list_for_each_entry_safe_from(pos, n, head, member)             \
-    for ((n) = list_entry((pos)->member.next, typeof(*(pos)), member);  \
-         &(pos)->member != (head);                                      \
-         (pos) = (n), (n) = list_entry((n)->member.next, typeof(*(n)), member))
+    for ( (n) = list_next_entry_or_null(head, pos, member);             \
+          (pos);                                                        \
+          (pos) = (n), (n) = list_next_entry_or_null(head, n, member) )
 
 /**
  * list_for_each_entry_safe_reverse
@@ -603,11 +637,11 @@  static inline void list_splice_init(struct list_head *list,
  * Iterate backwards over list of given type, safe against removal
  * of list entry.
  */
-#define list_for_each_entry_safe_reverse(pos, n, head, member)          \
-    for ((pos) = list_entry((head)->prev, typeof(*(pos)), member),      \
-         (n) = list_entry((pos)->member.prev, typeof(*(pos)), member);  \
-         &(pos)->member != (head);                                      \
-         (pos) = (n), (n) = list_entry((n)->member.prev, typeof(*(n)), member))
+#define list_for_each_entry_safe_reverse(pos, n, head, member)             \
+    for ( (pos) = list_last_entry_or_null(head, typeof(*(pos)), member),   \
+          (n) = (pos) ? list_prev_entry_or_null(head, pos, member) : NULL; \
+          (pos);                                                           \
+          (pos) = (n), (n) = list_prev_entry_or_null(head, n, member) )
 
 /**
  * list_for_each_rcu - iterate over an rcu-protected list
@@ -655,10 +689,10 @@  static inline void list_splice_init(struct list_head *list,
  * the _rcu list-mutation primitives such as list_add_rcu()
  * as long as the traversal is guarded by rcu_read_lock().
  */
-#define list_for_each_entry_rcu(pos, head, member)                      \
-    for ((pos) = list_entry((head)->next, typeof(*(pos)), member);      \
-         &rcu_dereference(pos)->member != (head);                       \
-         (pos) = list_entry((pos)->member.next, typeof(*(pos)), member))
+#define list_for_each_entry_rcu(pos, head, member)                        \
+    for ( (pos) = list_first_entry_or_null(head, typeof(*(pos)), member); \
+          rcu_dereference(pos);                                           \
+          (pos) = list_next_entry_or_null(head, pos, member) )
 
 /**
  * list_for_each_continue_rcu