LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
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>
Subject: Re: [GIT PULL] SafeSetID LSM changes for 5.4
Date: Mon, 23 Sep 2019 12:01:49 -0700
Message-ID: <CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com> (raw)
In-Reply-To: <CAJ-EccM49yBA+xgkR+3m5pEAJqmH_+FxfuAjijrQxaxxMUAt3Q@mail.gmail.com>

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

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-18 17:41 Micah Morton
2019-09-23 19:01 ` Linus Torvalds [this message]
2019-09-23 19:28   ` Linus Torvalds
2019-09-23 23:35     ` James Morris
2019-09-24  0:45       ` Linus Torvalds
2019-09-24  3:30         ` Micah Morton
2019-09-26 18:21           ` Linus Torvalds
2019-09-23 23:30   ` Paul E. McKenney
2019-09-24  0:44     ` Linus Torvalds
2019-10-21  6:58     ` Ingo Molnar
2019-10-22  2:13       ` Paul E. McKenney
2019-09-23 19:05 ` pr-tracker-bot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com' \
    --to=torvalds@linux-foundation.org \
    --cc=bart.vanassche@wdc.com \
    --cc=jannh@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=mortonm@chromium.org \
    --cc=paulmck@linux.vnet.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git