On 03/23/2018 09:14 AM, Stephen Smalley wrote: > On 03/23/2018 08:44 AM, Laurent Bigonville wrote: >> Le 23/03/18 à 13:26, Stephen Smalley a écrit : >>> On 03/23/2018 06:31 AM, Laurent Bigonville wrote: >>>> Le 22/03/18 à 17:09, Stephen Smalley a écrit : >>>>> On 03/21/2018 07:58 AM, Laurent Bigonville wrote: >>>>>> Hello, >>>>>> >>>>>> Could somebody have a quick look at the two patches that I opened for two dbus bugs: >>>>>> >>>>>> https://bugs.freedesktop.org/show_bug.cgi?id=92831 ; (stop using avc_init()) >>>>>> >>>>>> https://bugs.freedesktop.org/attachment.cgi?id=138021 ; (stop using selinux_set_mapping()) >>>>>> >>>>>> I'm also wondering whether the call to avc_add_callback() shouldn't be replaced by selinux_set_callback(), an opinion on this? >>>>> Patches look sane to me although I'm not really familiar with dbus code. >>>> Thanks for the review, Simon already had a look at the dbus part of the code >>>> >>>>> Looks like the callback is only used to trigger a reload of the dbus configuration (for dbus_contexts updates), and thus selinux_set_callback(SELINUX_CB_POLICYLOAD) is more appropriate than avc_add_callback(AVC_CALLBACK_RESET), since the latter is called upon setenforce 1 as well. However, if it were truly only for that purpose, one might argue that it ought to be a watch on the dbus_contexts file instead and not be tied to selinux at all. >>>> I really don't know the original rational of this. But I guess that if somebody is modifying dbus_contexts file, there are big chances that he will reload the policy as well(?). >>>> >>>> I'll change avc_add_callback() by selinux_set_callback(), we could say that as the file is in the SELinux path it's its responsibility. >>>> >>>>> NB This still won't fix the case where dbusd has already performed a string_to_security_class/av_perm lookup and the result has been cached by the libselinux class cache and then a subsequent policy update alters those values. That is what was fixed for systemd's usage of selinux_check_access() by selinux commit b408d72ca9104cb0c1bc4e154d8732cc7c0a9190. Offhand, I'm now wondering why I didn't just call flush_class_cache() from avc_reset() itself. That would fix it for other users of the AVC. You can't directly call flush_class_cache() from the dbus selinux policyload callback because it is hidden presently. If we can fix it directly in libselinux, then that is better. If not, we'd need to export it and probably give it a more unique name, ala selinux_flush_class_cache(). >>>> Right, that's a really good point, that I apparently overlooked... >>>> >>>> Is that cache really supposed to substantially speedup things? Would it be possible to create a version of selinux_check_access() that allows to pass a reference the cache or let selinux_check_access() create that cache itself? If it's the case I guess that dbus-broker would benefit of that as well as they are using selinux_check_access(). >>>> >>>> Otherwise we can indeed clean up the cache our self, but wasn't the goal of selinux_check_access() to be an "easy" interface to use, asking the applications to do this kind of housekeeping is defeating that purpose, isn't it? >>> If you use selinux_check_access(), then the class cache is already flushed for you upon an AVC reset; that is what the commit I referenced above did. The problem in the case of dbusd is that it is not using selinux_check_access() but rather its own direct usage of string_to_security_class/av_perm() and avc_has_perm(). That's why we need to either take the call to flush_class_cache() in libselinux to avc_reset() so that it is done for all users of the AVC, or we need to export it and have dbusd call it from its policy reload callback. >> >> No, I meant the decision cache used by avc_has_perm(). dbus is not using selinux_check_access() because there is no way to set that decision cache (the 5th parameter of avc_has_perm() is NULL) > > Oh, you mean the AVC entry reference. Usually that would be cached in a per-object structure of some sort, e.g. in the kernel, there used to be an AVC entry reference cached in the task, inode, and other security structures. However, the AVC entry references were dropped from the kernel altogether when the kernel AVC was converted to using RCU. They are still used in the libselinux AVC, but I wouldn't expect dbusd is gaining much from using it especially since dbusd is only storing a single reference globally rather than any kind of per-object one. The larger cost of switching to using selinux_check_access() is probably the avc_context_to_sid() lookups since you already have the SIDs available to you. > > You don't necessarily have to switch to selinux_check_access(). But we do need to ensure that the class cache is flushed, either by adding a call to avc_reset() in libselinux if that is possible, or by exporting flush_class_cache() and calling it from the dbusd policy reload callback. Sorry, to be clear, the first option is to add a call to flush_class_cache() from avc_reset() in libselinux, so that it is always called upon an AVC reset (and dropping the custom avc reset callback from selinux_check_access).
