Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <CAJ-EccM49yBA+xgkR+3m5pEAJqmH_+FxfuAjijrQxaxxMUAt3Q@mail.gmail.com>
In-Reply-To: <CAJ-EccM49yBA+xgkR+3m5pEAJqmH_+FxfuAjijrQxaxxMUAt3Q@mail.gmail.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Mon, 23 Sep 2019 12:01:49 -0700
Message-ID: <CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com>
Subject: Re: [GIT PULL] SafeSetID LSM changes for 5.4
To:     Micah Morton <mortonm@chromium.org>, Jann Horn <jannh@google.com>,
        Bart Van Assche <bart.vanassche@wdc.com>,
        "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-security-module <linux-security-module@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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