Messages in this thread | | | From | Linus Torvalds <> | Date | Mon, 23 Sep 2019 12:01:49 -0700 | Subject | Re: [GIT PULL] SafeSetID LSM changes for 5.4 |
| |
On Wed, Sep 18, 2019 at 10:41 AM Micah Morton <mortonm@chromium.org> wrote: > > Fix for SafeSetID bug that was introduced in 5.3
So this seems to be a good fix, but the bug itself came from the fact that
rcu_swap_protected(..)
is so hard to read, and I don't see *why* it's so pointlessly hard to read.
Yes, we have some macros that change their arguments, but they have a _reason_ to do so (ie they return two different values) and they tend to be very special in other ways too.
But rcu_swap_protected() has no reason for it's odd semantics.
Looking at that 'handle_policy_update()' function, it's entirely reasonable to think "pol cannot possibly be NULL". When I looked at the fix patch, that was my initial reaction too, and it's probably the reason Jann's commit 03638e62f55f ("LSM: SafeSetID: rewrite userspace API to atomic updates") had that bug to begin with.
I don't see the original discussion at all, it's not on Linux-Security-Module for some reason, so I can't tell when/if the NULL pointer test got deleted.
Anyway, this bug would likely had been avoided if rcu_swap_protected() just returned the old pointer instead of changing the argument.
There are only a handful or users of that macro, maybe this could be fixed?
Adding some of the RCU parties to the participants..
Also, the commit message for this fix was a mess, I feel. It says "SafeSetID: Stop releasing uninitialized ruleset", but the ruleset it releases is perfectly initialized. It just might be NULL because it doesn't _exist_.
Linus
| |