Re: [RFC PATCH] selinux: move policy commit after updating selinuxfs

[Daniel Burgener <dburgener@linux.microsoft.com>]

On 8/5/20 8:35 AM, Stephen Smalley wrote:
> On 8/4/20 4:51 PM, Daniel Burgener wrote:
>
>> On 8/4/20 9:53 AM, Stephen Smalley wrote:
>>> With the refactoring of the policy load logic in the security
>>> server from the previous change, it is now possible to split out
>>> the committing of the new policy from security_load_policy() and
>>> perform it only after successful updating of selinuxfs. Change
>>> security_load_policy() to return the newly populated policy
>>> data structures to the caller, export selinux_policy_commit()
>>> for external callers, and introduce selinux_policy_cancel() to
>>> provide a way to cancel the policy load in the event of an error
>>> during updating of the selinuxfs directory tree.  Further, rework
>>> the interfaces used by selinuxfs to get information from the policy
>>> when creating the new directory tree to take and act upon the
>>> new policy data structure rather than the current/active policy.
>>> Update selinuxfs to use these updated and new interfaces. While
>>> we are here, stop re-creating the policy_capabilities directory
>>> on each policy load since it does not depend on the policy, and
>>> stop trying to create the booleans and classes directories during
>>> the initial creation of selinuxfs since no information is available
>>> until first policy load.
>>>
>>> After this change, a failure while updating the booleans and class
>>> directories will cause the entire policy load to be canceled, leaving
>>> the original policy intact, and policy load notifications to userspace
>>> will only happen after a successful completion of updating those
>>> directories.  This does not (yet) provide full atomicity with respect
>>> to the updating of the directory trees themselves.
>>
>> I have a patch series to perform the atomic updates very close to 
>> done, using vfs_rename with RENAME_EXCHANGE to update the directories 
>> out of tree and then swap them in as discussed earlier.  I've just 
>> been doing some final style cleanup before sending to the list.  I'll 
>> need to rebase on top of these changes of course.  I didn't touch any 
>> of the error recovery portions, so I hope my series will complement 
>> this patch nicely.
>
> Great, I was trying to ensure that we wouldn't conflict/overlap 
> significantly.
>
>>> This patch is relative to my previous one,
>>> https://patchwork.kernel.org/patch/11698505/. Although this does
>>> not ensure atomicity when updating the selinuxfs directoty tree,
>>> I suspect it will solve Daniel's original bug because systemd/dbusd
>>> won't get the policy load notifications until the kernel is done
>>> updating selinuxfs and therefore won't try to re-read selinuxfs
>>> in the middle of it (because libselinux caches the class/perm
>>> mappings and only flushes on a reload).
>> I agree with your suspicion that this will resolve the bug we've been 
>> seeing (although only as a result of changing the timing, as you 
>> point out).  Thanks for your work on this!
>
> If you can easily test that my patches resolve that bug for you, you 
> could add a Tested-by tag.  One caveat is that it sounds like I'll be 
> making one more change to the previous patch per Ondrej's request to 
> avoid taking the read lock around sidtab_convert().
Sounds good.  I will wait for your update, and then test this series.
>
>>>   @@ -563,15 +560,19 @@ static ssize_t sel_write_load(struct file 
>>> *file, const char __user *buf,
>>>       if (copy_from_user(data, buf, count) != 0)
>>>           goto out;
>>>   -    length = security_load_policy(fsi->state, data, count);
>>> +    length = security_load_policy(fsi->state, data, count, 
>>> &newpolicy);
>>>       if (length) {
>>>           pr_warn_ratelimited("SELinux: failed to load policy\n");
>>>           goto out;
>>>       }
>>>   -    length = sel_make_policy_nodes(fsi);
>>> -    if (length)
>>> +    length = sel_make_policy_nodes(fsi, newpolicy);
>>> +    if (length) {
>>> +        selinux_policy_cancel(fsi->state, newpolicy);
>>>           goto out1;
>> As things stand as of this patch, I think that this means that in the 
>> event of a failure in recreating the directories, that directory will 
>> be left unpopulated or partially populated.  We could even get in a 
>> state where the booleans directory has already been updated to the 
>> new policy and the class directory has not. The full solution is of 
>> course atomic swapover, which as I mentioned above I hope to submit a 
>> series for soon, but I wonder if recreating the directories on the 
>> old policy would be a better interim state?  That probably depends on 
>> what sorts of errors are possible.  If we've failed because of 
>> something about the new policy, recreating the old directories should 
>> get us back to a good state.  If we can't create new directories at 
>> all for whatever reason, trying to recreate might leave us worse off 
>> than before we started.
>
> I deliberately avoided any changes to the error handling during 
> re-creation of the booleans and class directories because I viewed 
> that as logically separate from my change and likely to conflict with 
> your changes.  So I expect to revisit that issue after both my patches 
> and yours land. I think the only scenario where 
> sel_make_bools/classes() can fail is an out-of-memory condition and if 
> we are out of memory then we are unlikely to be able to re-create the 
> old directories/files again.  Hence, I don't think there is anything 
> useful we can do without the atomic swapover. At most, we can delete 
> everything under booleans and class on any failure while re-creating 
> so that we aren't left with the partial set of booleans/classes.
>
> The other possibility I considered is explicitly checking whether 
> there are any changes to booleans or classes between the old and new 
> policies and if not, skipping that part of the selinuxfs update.  That 
> however would require a new security server function to iterate over 
> all of the booleans and classes in two selinux_policy structures and 
> compare them for equality.  Didn't seem worth it if the atomic 
> swapover support was coming anyway.
>
Sounds good.

-Daniel