[systemd-devel] [PATCH v3 2/2] selinux: fix unnecessary generic SELinux check due to unit objects in UNIT_NOT_FOUND

[HATAYAMA Daisuke]

systemd creates a unit object of A.service when it is referenced in
various contexts such as that systemd parses a unit file and finds a
dependency, like After=A.service, in some unit file or via systemd is
requested a D-Bus operation such as systemctl status A.service, and
then registers it in manager->units hash table.

If the referenced unit file actually doesn't exist, load state of the
unit object becomes UNIT_NOT_FOUND and is left in manager->units until
it is cleaned up at the next manager loop.

The problem here is that mac_selinux_unit_access_check_strv()
overlooks the situation that some unit object of A.service in
UNIT_NOT_FOUND could still be left in the manager->units hash table
but a new unit file A.service has just been created. Then, the unit
object in UNIT_NOT_FOUND state hides presence of the A.service. As a
result, we don't lead to an SELinux unit access check with a SELinux
context label of the A.service.

To fix this, if manager_load_unit() returns a unit object in
UNIT_NOT_FOUND from the manager->units hash table, clean up the unit
object and retry loading unit files.
---
 src/core/manager.c        |    4 ++--
 src/core/manager.h        |    2 ++
 src/core/selinux-access.c |   14 ++++++++++++++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/src/core/manager.c b/src/core/manager.c
index a1c5433..d05d6fb 100644
--- a/src/core/manager.c
+++ b/src/core/manager.c
@@ -771,7 +771,7 @@ static int manager_connect_bus(Manager *m, bool reexecuting) {
         return bus_init(m, try_bus_connect);
 }
 
-static unsigned manager_dispatch_cleanup_queue(Manager *m) {
+unsigned manager_dispatch_cleanup_queue(Manager *m) {
         Unit *u;
         unsigned n = 0;
 
@@ -847,7 +847,7 @@ good:
         u->gc_marker = gc_marker + GC_OFFSET_GOOD;
 }
 
-static unsigned manager_dispatch_gc_queue(Manager *m) {
+unsigned manager_dispatch_gc_queue(Manager *m) {
         Unit *u;
         unsigned n = 0;
         unsigned gc_marker;
diff --git a/src/core/manager.h b/src/core/manager.h
index 4ef869d..bea7a17 100644
--- a/src/core/manager.h
+++ b/src/core/manager.h
@@ -322,6 +322,8 @@ void manager_dump_jobs(Manager *s, FILE *f, const char *prefix);
 
 void manager_clear_jobs(Manager *m);
 
+unsigned manager_dispatch_cleanup_queue(Manager *m);
+unsigned manager_dispatch_gc_queue(Manager *m);
 unsigned manager_dispatch_load_queue(Manager *m);
 
 int manager_environment_add(Manager *m, char **minus, char **plus);
diff --git a/src/core/selinux-access.c b/src/core/selinux-access.c
index f52bc6d..5921898 100644
--- a/src/core/selinux-access.c
+++ b/src/core/selinux-access.c
@@ -292,10 +292,24 @@ int mac_selinux_unit_access_check_strv(char **units,
         int r;
 
         STRV_FOREACH(i, units) {
+        retry:
                 r = manager_load_unit(m, *i, NULL, error, &u);
                 if (r < 0)
                         return r;
                 if (u) {
+                        /* Clean up an unit object in UNIT_NOT_FOUND
+                         * load state if it's still left in m->units,
+                         * and then retry to load a given unit file.
+                         * Give up UNIT_ERROR case to aovid such as
+                         * infinite loop due to ENOMEM.
+                         */
+                        if (r == 1 && u->load_state == UNIT_NOT_FOUND) {
+                                unit_add_to_gc_queue(u);
+                                manager_dispatch_gc_queue(m);
+                                unit_add_to_cleanup_queue(u);
+                                manager_dispatch_cleanup_queue(m);
+                                goto retry;
+                        }
                         r = mac_selinux_unit_access_check(u, message, permission, error);
                         if (r < 0)
                                 return r;