Reload config on SELinux policy load

[matt-sheets]

When a new SELinux policy is loaded, the dbus config file it carries may have been updated as well. As such, we should reload the dbus configuration to catch any changes. This logic already exists in the dbus-daemon code and this change is largely based off of that code.

[dvdhrm]

Thanks for the PR! Sadly, we cannot rely on the broker to have its controller as parent PID. So this either needs channeling via the controller interface, or we simply make the launcher listen for these events and trigger the reload from there (which I think is preferable).

[matt-sheets]

Thanks for the feedback, I am not very familiar with the broker code, so any input is more than welcome.

I have tried to move the callback registration into launcher itself, but the actual callback method never seems to run. I am still digging into this but not making great progress.

Could you possibly point me in the correct direction when you say "channeling via the controller interface" if setting up the callback in the launcher doesn't work out?

[matt-sheets]

I have dug more and have come up with the following:

The reason why the launcher was not seeing the callback run and the broker was, is that the broker will call selinux_check_access which intern calls selinux_status_updated, which triggers the callback. The launcher has no such logic. As such I have come up with a few options for folks who know more than me to consider:

1. Add a polling method to the launcher to periodically call selinux_status_updated. This is probably the worst option but the most straightforward in terms of complexity.
2. Add an inotify on the selinux status file to call selinux_status_updated on a change. This will require additional SELinux policy to work in enforcing, specifically allowing the broker process to watch the file. Also, I noticed there was already some dir watching going on in dirwatch.c. We may be able to plug into that, but I got a bit lost in which actual process was watching what.
3. We signal the launcher to reload via a dbus message from the broker. I think I can do this from the broker code by calling controller_send_dbus_reload directly. But we get into a weird situation with registering the callback and having access to what that function needs in the call back. I have some ideas that should work but none of them are pretty.

[matt-sheets]

@dvdhrm Hi just a gentle ping about the proposed paths I laid out in my last comment.

[dvdhrm]

Thanks for the investigation. I still believe that monitoring operations like this belong into the launcher, especially if they are asynchronous and do not necessarily require tight ordering with other calls.

Sadly, the kernel does not support wait-queues on /selinux/status, so we cannot poll(2) it. Maybe it is worth adding it to the kernel? I see no reason not to.

Anyway, in the meantime I see 2 solutions:

1. Make the launcher check /selinux/status on each event-loop iteration (selinux_status_udpated(3)). In the fast-path this is just an integer load+comparison, so really negligible. This has the downside that it can stall for a long time if there is no traffic to the launcher. I don't want any idle/poll wakeups, unless we have no other idea.

2. Make the broker send a new signal to the launcher, which basically says "selinux status updated". The launcher can then react to it accordingly. We already do something similar with the activation-env, see broker_update_environment() and controller_dbus_send_environment().

Does that sound feasible?

[dvdhrm]

Btw., your policy_reload_callback() will not have any context information, since SELinux does not allow us to pass it.

A very simple solution would be to call raise(SIGHUP) and make broker/broker.c extend the signalfd to handle SIGHUP, detect it in broker_dispatch_signals() and there call a new broker_propagate_sighup(), which itself calls into a new controller_dbus_send_sighup(), which then sends a new signal org.bus1.DBus.Broker.Sighup, which is then handled by the launcher.

This generic "sighup" could then be used by other parts of the infrastructure as well, to basically say "something changed, the launcher should check any status pages". It would even allow manually sending SIGHUP to the process, for debugging/introspection to trigger the update.

[matt-sheets]

@dvdhrm Thanks for the input, and sorry this took me so long to get back around to. I have pushed some changes to align with your last suggestion, but I think I am messing things up in controller_dbus_send_sighup. I copied almost all of the code from the other controller_dbus_send messages but don't really understand what to do with c_dvar related code. When I try to test things my desktop session dies when I call load_policy to trigger things, which has made debugging difficult. Please let me know if I am doing something obviously dumb.

[dvdhrm]

Thanks for the followup, see my inline comments.

[dvdhrm]

You probably want to propagate the return value of this call via error_fold(). You currently fall-through to DISPATCH_EXIT, which will terminate the broker (which likely is what you are seeing in your tests).

[dvdhrm]

What does SELinux do with return values from this callback?

[matt-sheets]

In our case nothing. If the callback returns non-zero selinux_status_updated returns -1, but the dbus-broker code isn't calling selinux_status_updated directly. Its calling it through selinux_check_access which ignores the return.

[dvdhrm]

Suggested change

	return controller_dbus_send_sighup(&broker->controller);
	int r;
	r = controller_dbus_send_sighup(&broker->controller);
	return error_fold(r);