All three implementations of rte_eal_alarm_cancel() free entries while
walking the alarm list with LIST_FOREACH, which leaves the iterator
pointing into freed memory.
Linux and FreeBSD use two loops: one draining matches from the head of
the list, then a LIST_FOREACH over the rest that frees the current
entry and assigns the saved ap_prev to ap so iteration resumes from the
predecessor. ap_prev is only refreshed to a live entry by an iteration
that does not remove, and the head loop leaves it NULL when it empties
the list. A removal in the second loop then sets ap to NULL or to an
already freed entry, and the LIST_FOREACH increment dereferences it.
GCC -fanalyzer reports the freed case:
lib/eal/linux/eal_alarm.c:224:44: warning: use after 'free' of 'ap'
[CWE-416] [-Wanalyzer-use-after-free]
Windows has no such dance: it calls alarm_remove_unsafe() straight from
the loop body, so the increment reads freed memory on every removal but
the last.
Replace all of these with LIST_FOREACH_SAFE. FreeBSD sys/queue.h and
the bundled Windows sys/queue.h already provide it; glibc does not, so
define it locally as is already done in several drivers.
Fixes: af75078fece3 ("first public release")
Fixes: f4cbdbc7fbd2 ("eal/windows: implement alarm API")
Cc: [email protected]
Signed-off-by: Stephen Hemminger <[email protected]>
---
lib/eal/freebsd/eal_alarm.c | 41 +++++++------------------------------
lib/eal/linux/eal_alarm.c | 40 ++++++++++++------------------------
lib/eal/windows/eal_alarm.c | 4 ++--
3 files changed, 22 insertions(+), 63 deletions(-)
diff --git a/lib/eal/freebsd/eal_alarm.c b/lib/eal/freebsd/eal_alarm.c
index c03e281e67..1585a651e9 100644
--- a/lib/eal/freebsd/eal_alarm.c
+++ b/lib/eal/freebsd/eal_alarm.c
@@ -264,7 +264,7 @@ RTE_EXPORT_SYMBOL(rte_eal_alarm_cancel)
int
rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
{
- struct alarm_entry *ap, *ap_prev;
+ struct alarm_entry *ap, *ap_next;
int count = 0;
int err = 0;
int executing;
@@ -277,15 +277,12 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void
*cb_arg)
do {
executing = 0;
rte_spinlock_lock(&alarm_list_lk);
- /* remove any matches at the start of the list */
- while (1) {
- ap = LIST_FIRST(&alarm_list);
- if (ap == NULL)
- break;
- if (cb_fn != ap->cb_fn)
- break;
- if (cb_arg != ap->cb_arg && cb_arg != (void *) -1)
- break;
+
+ LIST_FOREACH_SAFE(ap, &alarm_list, next, ap_next) {
+ if (cb_fn != ap->cb_fn ||
+ (cb_arg != (void *)-1 && cb_arg !=
ap->cb_arg))
+ continue;
+
if (ap->executing == 0) {
LIST_REMOVE(ap, next);
free(ap);
@@ -301,31 +298,7 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void
*cb_arg)
executing++;
else
err = EINPROGRESS;
-
- break;
- }
- }
- ap_prev = ap;
-
- /* now go through list, removing entries not at start */
- LIST_FOREACH(ap, &alarm_list, next) {
- /* this won't be true first time through */
- if (cb_fn == ap->cb_fn &&
- (cb_arg == (void *)-1 ||
- cb_arg == ap->cb_arg)) {
- if (ap->executing == 0) {
- LIST_REMOVE(ap, next);
- free(ap);
- count++;
- ap = ap_prev;
- } else if (pthread_equal(ap->executing_id,
- pthread_self()) == 0) {
- executing++;
- } else {
- err = EINPROGRESS;
- }
}
- ap_prev = ap;
}
rte_spinlock_unlock(&alarm_list_lk);
diff --git a/lib/eal/linux/eal_alarm.c b/lib/eal/linux/eal_alarm.c
index a1433eb867..eb41064851 100644
--- a/lib/eal/linux/eal_alarm.c
+++ b/lib/eal/linux/eal_alarm.c
@@ -25,6 +25,13 @@
#define TFD_NONBLOCK O_NONBLOCK
#endif
+#ifndef LIST_FOREACH_SAFE
+#define LIST_FOREACH_SAFE(var, head, field, tvar) \
+ for ((var) = LIST_FIRST((head)); \
+ (var) && ((tvar) = LIST_NEXT((var), field), 1); \
+ (var) = (tvar))
+#endif
+
#define NS_PER_US 1000
#define US_PER_MS 1000
#define MS_PER_S 1000
@@ -206,7 +213,7 @@ RTE_EXPORT_SYMBOL(rte_eal_alarm_cancel)
int
rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
{
- struct alarm_entry *ap, *ap_prev;
+ struct alarm_entry *ap, *ap_next;
int count = 0;
int err = 0;
int executing;
@@ -219,10 +226,11 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void
*cb_arg)
do {
executing = 0;
rte_spinlock_lock(&alarm_list_lk);
- /* remove any matches at the start of the list */
- while ((ap = LIST_FIRST(&alarm_list)) != NULL &&
- cb_fn == ap->cb_fn &&
- (cb_arg == (void *)-1 || cb_arg == ap->cb_arg))
{
+
+ LIST_FOREACH_SAFE(ap, &alarm_list, next, ap_next) {
+ if (cb_fn != ap->cb_fn ||
+ (cb_arg != (void *)-1 && cb_arg !=
ap->cb_arg))
+ continue;
if (ap->executing == 0) {
LIST_REMOVE(ap, next);
@@ -236,29 +244,7 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void
*cb_arg)
executing++;
else
err = EINPROGRESS;
-
- break;
- }
- }
- ap_prev = ap;
-
- /* now go through list, removing entries not at start */
- LIST_FOREACH(ap, &alarm_list, next) {
- /* this won't be true first time through */
- if (cb_fn == ap->cb_fn &&
- (cb_arg == (void *)-1 || cb_arg ==
ap->cb_arg)) {
-
- if (ap->executing == 0) {
- LIST_REMOVE(ap, next);
- free(ap);
- count++;
- ap = ap_prev;
- } else if (pthread_equal(ap->executing_id,
pthread_self()) == 0)
- executing++;
- else
- err = EINPROGRESS;
}
- ap_prev = ap;
}
rte_spinlock_unlock(&alarm_list_lk);
diff --git a/lib/eal/windows/eal_alarm.c b/lib/eal/windows/eal_alarm.c
index 0b11d331dc..ed6e7f2245 100644
--- a/lib/eal/windows/eal_alarm.c
+++ b/lib/eal/windows/eal_alarm.c
@@ -190,7 +190,7 @@ RTE_EXPORT_SYMBOL(rte_eal_alarm_cancel)
int
rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
{
- struct alarm_entry *ap;
+ struct alarm_entry *ap, *ap_next;
unsigned int state;
int removed;
bool executing;
@@ -207,7 +207,7 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void
*cb_arg)
rte_spinlock_lock(&alarm_lock);
- LIST_FOREACH(ap, &alarm_list, next) {
+ LIST_FOREACH_SAFE(ap, &alarm_list, next, ap_next) {
if (!alarm_matches(ap, cb_fn, cb_arg))
continue;
--
2.53.0
