ulogd2: avoid use after free in unregister on global ulogd_fds linked list

Invalid read of size 4
   at 0x405F60: ulogd_unregister_fd (select.c:74)
   by 0x4E4E3DF: ??? (in /usr/lib/ulogd/ulogd_inppkt_NFLOG.so)
   by 0x405003: stop_pluginstances (ulogd.c:1335)
   by 0x405003: sigterm_handler_task (ulogd.c:1383)
   by 0x405153: call_signal_handler_tasks (ulogd.c:424)
   by 0x405153: signal_channel_callback (ulogd.c:443)
   by 0x406163: ulogd_select_main (select.c:105)
   by 0x403CF3: ulogd_main_loop (ulogd.c:1070)
   by 0x403CF3: main (ulogd.c:1649)

Problem is that ulogd_inppkt_NFLOG.c::stop() calls ulogd_unregister_fd()
which does llist_del().  This llist_del may touch ->prev pointer.

As the list element is in private data, we cannot do this llist_del
from stop_pluginstances().

Therefore, the free() process moved location after finishing ulogd_unregister_fd().

Signed-off-by: Kyuwon Shim <kyuwon.shim@alliedtelesis.co.nz>
Signed-off-by: Florian Westphal <fw@strlen.de>

diff --git a/src/ulogd.c b/src/ulogd.c
index 8ea9793ec0fb608a9d9d8511e8a25cecc967b7b0..6c5ff9ac7a515493da577ab6140f78769c9d5e91 100644 (file)
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -1334,6 +1334,15 @@ static void stop_pluginstances()
                                (*pi->plugin->stop)(pi);
                                pi->private[0] = 0;
                        }
+
+                       /* NB: plugin->stop() might access other plugin instances,
+                        * so we cannot free right away.
+                        */
+               }
+       }
+
+       llist_for_each_entry(stack, &ulogd_pi_stacks, stack_list) {
+               llist_for_each_entry_safe(pi, npi, &stack->list, list) {
                        free(pi);
                }
        }