On Tue, Jul 14, 2020 at 5:35 PM Stephen Smalley <stephen.smalley.work@xxxxxxxxx> wrote: > > On Tue, Jul 14, 2020 at 5:20 PM Mike Palmiotto > <mike.palmiotto@xxxxxxxxxxxxxxx> wrote: > > > > On Tue, Jul 14, 2020 at 5:03 PM Stephen Smalley > > <stephen.smalley.work@xxxxxxxxx> wrote: > > > <snip> > > In its current form, your patch replaces one call in > avc_has_perm_noaudit() to avc_netlink_check_nb() with a call to > selinux_status_updated(). With that change alone, for existing > callers of avc_has_perm_noaudit() that do not themselves call > selinux_status_open(), selinux_status_updated() will find that the > status page is NULL and will return immediately with an error. It > won't fall back to probing the netlink socket. So you either need to > test for an error return and fall back yourself to > avc_netlink_check_nb(), or change selinux_status_updated() to do that, > or replace the use of avc_netlink_open/loop/close in avc.c with > selinux_status_open/close. Likewise for selinux_check_access(). For some reason I completely glossed over the selinux_status == NULL case. I guess I didn't see errors when testing dbus because verbose logging wasn't turned on. I'll take a look at what it would take to move over sestatus entirely and if it's too tricky/error-prone, _I think_ it should be fine to move the selinux_status == NULL check down with the MAP_FAILED case so they both fall through to avc_netlink_check_nb(). > > > Do you think we should go ahead and completely swap in sestatus? I was > > just worried about breaking userspace object managers that are > > currently using netlink threads by default, for instance > > systemd-dbusd. I can spend some more time getting those to work with > > the status page if you think that's worthwhile. > > I'd be interested in understanding the impact of such a change on > existing userspace object managers. If we can switch the default > behavior for applications that are not explicitly using > avc_netlink_*() interfaces themselves (e.g. they are only using > selinux_check_access or avc_has_perm), then that would be beneficial > since I think it fully removes the need for a system call on the AVC > cache-hit code path. I'll have to do a bit of digging to see how this will affect dbus, et al. On first blush, it looks like they're just doing avc_netlink_check_nb() in their watch thread. Presumably other object managers are doing something similar so we would just need to make sure there is a netlink fd available. > > > > Finally, I don't think you need to sanitize > > > the enforcing value from the kernel; it takes care of that itself > > > these days and no point in fixing it up for old kernels now. > > > > Very good to know, thanks! I'll update in the next version. -- Mike Palmiotto https://crunchydata.com
