Re: linux-next: build warning in Linus' tree

[Micah Morton]

On Tue, Oct 27, 2020 at 8:28 PM Stephen Rothwell  wrote:
>
> Hi all,
>
> In Linus' tree, today's linux-next build (htmldocs) produced
> this warning:
>
> Documentation/admin-guide/LSM/SafeSetID.rst:110: WARNING: Title underline too 
> short.

Thanks for the heads up. I think someone sent a patch to fix this
yesterday: https://marc.info/?l=linux-doc=160379233902729=2

Do I need to do anything else or should that cover it?

>
> Introduced by commit
>
>   5294bac97e12 ("LSM: SafeSetID: Add GID security policy handling")
>
> --
> Cheers,
> Stephen Rothwell

Re: [GIT PULL] SafeSetID changes for v5.10

[Micah Morton]

On Sun, Oct 25, 2020 at 10:54 AM Linus Torvalds
 wrote:
>
> On Fri, Oct 23, 2020 at 12:15 PM Micah Morton  wrote:
> >
> > Ok so before the rebase ("reparent"), the commits were based on top of
> > some commit that was months old at this point (can't quite remember
> > now, I think one of the -rc's for v5.8).
>
> Nobody cares if the old parent is old. In fact, that's usually a good
> sign that the code has had testing and is changing things that aren't
> in flux for other reasons.

Ok thanks for the explanation, I think this was the most important
piece I was missing, but makes sense now.

>
> It's often a good idea to make a test-merge and verify that things are
> ok, but that's for your _personal_ verification, and shouldn't be
> something that anybody else sees.
>
> And even with a test-merge, it doesn't matter if there is some simple
> conflict - we have those all the time, and conflicts aren't bad. In
> fact, they allow me to see "ok, things have changed here in parallel",
> and I'll be aware of it.
>
> The main reason to rebase is if things have changed _so_ much that you
> really need to re-do things, or if there is some major bug in _your_
> branch that simply needs to be fixed.

Yeah sounds good, I'll just get in the habit of doing a test merge and
note in the pull request whether there are any conflicts or not.

>
> >So I had basically considered it
> > a no-op rebase. I probably should have explained this in the pull
> > request.
>
> If it's a no-op rebase, thern DON'T DO IT. Really. It just means that
> now you have lost all the testing.
>
> Thinking that it's a no-op doesn't really help. No bugs are
> _intentional_, I would seriously hope. Lack of testing is lack of
> testing, regardless of whether you think it would not matter.
>
> It also destroys the real history of the code, which is sad.
>
> Now, sometimes you may _want_ to destroy the real history of the code
> (as in "Oh, this history is too ugly to survive, and makes bisection
> impossible because some of the intermediate state was seriously
> buggy"). That is then one of those few valid reasons to rebase (see
> the "major bug in your branch" case above).
>
> But 99% of the time, rebasing is bad. If it was in linux-next and
> there were no horrible problems with it, and it got tested there, then
> just leave it alone and don't destroy the testing it did get.
>
> Anyway, I've pulled this now, but honestly, don't do this again. Stop
> rebasing without a big and immediate reason, and stop destroying
> whatever testing it got in linux-next.

Got it.

>
> And if you _do_ rebase, and you _do_ have a real and very serious
> reason, then mention that reason and explain it. But no "the rebase
> didn't make any difference" isn't a reason. Quite the reverse.
>
>Linus
>
>Linus

Re: [GIT PULL] SafeSetID changes for v5.10

[Micah Morton]

On Fri, Oct 23, 2020 at 10:57 AM Linus Torvalds
 wrote:
>
> On Thu, Oct 15, 2020 at 5:01 PM Micah Morton  wrote:
> >
> > I just rebased to v5.9 to make sure the 1-line changes that touch
> > kernel/capability.c, kernel/groups.c and kernel/sys.c still applied
> > cleanly without conflicts. Should I have rebased onto one of the -rc's
> > for v5.9 instead?
>
> No. You shouldn't have rebased at all.

Ok so before the rebase ("reparent"), the commits were based on top of
some commit that was months old at this point (can't quite remember
now, I think one of the -rc's for v5.8).

I guess the lesson here is never rebase or fast-forward merge my
upstream-bound -next branch until it has emptied entirely into the
mainline? And if that doesn't happen for whatever reason during one
merge window and I have to wait for the next one, I just send you
un-reparented/un-fastforwarded possibly outdated commits and you will
resolve conflicts if any?

The reason for the rebase making sense to me here was that the changes
to common kernel code are very simple (a few one line changes) and
easy to quickly verify after the rebase -- and the vast majority of
the complexity of the code in the pull request was confined to the
SafeSetID code base, which had no changes over the time span from
original base to the reparented base. So I had basically considered it
a no-op rebase. I probably should have explained this in the pull
request.

Thanks

>
> Making sure something applies cleanly is simply not a reason to rebase.
>
> See
>
>   Documentation/maintainer/rebasing-and-merging.rst
>
> for some common rules.
>
>  Linus

Re: [GIT PULL] SafeSetID changes for v5.10

[Micah Morton]

On Thu, Oct 15, 2020 at 4:06 PM Linus Torvalds
 wrote:
>
> These were rebased since the merge window started, for no apparent reason.
>
> Were they in linux-next?

Yeah, they are changes that were originally targeting the v5.9 merge
window (and thus were in -next during July/August) but I didn't get
the chance to send a pull request for them. Since I didn't touch my
-next branch since then they are also in 'next-20201013' and
'next-20201015'.

I just rebased to v5.9 to make sure the 1-line changes that touch
kernel/capability.c, kernel/groups.c and kernel/sys.c still applied
cleanly without conflicts. Should I have rebased onto one of the -rc's
for v5.9 instead?

>
> And if so, why was I sent some different version?
>
>  Linus

[GIT PULL] SafeSetID changes for v5.10

[Micah Morton]

The following changes since commit bbf5c979011a099af5dc76498918ed7df445635b:

  Linux 5.9 (2020-10-11 14:15:50 -0700)

are available in the Git repository at:

  https://github.com/micah-morton/linux.git tags/safesetid-5.10

for you to fetch changes up to 03ca0ec138927b16fab0dad7b869f42eb2849c94:

  LSM: SafeSetID: Fix warnings reported by test bot (2020-10-13 09:17:36 -0700)


SafeSetID changes for v5.10

The changes in this pull request are mostly contained to within the
SafeSetID LSM, with the exception of a few 1-line changes to change
some ns_capable() calls to ns_capable_setid() -- causing a flag
(CAP_OPT_INSETID) to be set that is examined by SafeSetID code and
nothing else in the kernel. These changes have been baking in -next and
actually were in -next for the entire v5.9 merge window but I didn't
have a chance to send them.

The changes to SafeSetID internally allow for setting up GID transition
security policies, as already existed for UIDs.

NOTE: I'm re-using my safesetid-next branch here as the branch for
creating the pull request. I think that's fine, not sure if this is the
normal workflow or not. Also, I use 'git rebase vX.X' to put my commits
on top of the latest stable release. Again, I verified with gitk that I
don't have any weird history in my branch that will mess things up so
AFAICT that should be fine too.



Thomas Cedeno (3):
  LSM: Signal to SafeSetID when setting group IDs
  LSM: SafeSetID: Add GID security policy handling
  LSM: SafeSetID: Fix warnings reported by test bot

 Documentation/admin-guide/LSM/SafeSetID.rst |  29 +++--
 kernel/capability.c |   2 +-
 kernel/groups.c |   2 +-
 kernel/sys.c|  10 +-
 security/safesetid/lsm.c| 190 +---
 security/safesetid/lsm.h|  38 --
 security/safesetid/securityfs.c | 190 
 7 files changed, 336 insertions(+), 125 deletions(-)

Re: [GIT PULL] SafeSetID LSM changes for v5.8

[Micah Morton]

On Mon, Jun 15, 2020 at 11:03 AM James Morris  wrote:
>
> On Mon, 15 Jun 2020, Micah Morton wrote:
>
> > On Sun, Jun 14, 2020 at 10:21 PM James Morris  wrote:
> > >
> > > On Sun, 14 Jun 2020, Micah Morton wrote:
> > >
> > > > This patch was sent to the security mailing list and there were no 
> > > > objections.
> > >
> > > Standard practice for new or modified LSM hooks is that they are reviewed
> > > and acked by maintainers of major LSMs (SELinux, AppArmor, and Smack, at
> > > least).
> > >
> > > "No objections" should be considered "not reviewed".
> > >
> > > Can you add your tree to linux-next?
> > > https://www.kernel.org/doc/man-pages/linux-next.html
> >
> > Sure, I can do that. I should just send an email to Stephen Rothwell
> > asking him to include the -next branch from my tree?
>
> Yep, thanks.

The commit is in -next as of next-20200615

Thanks

>
> >
> > >
> > > --
> > > James Morris
> > > 
> > >
> >
>
> --
> James Morris
> 
>

Re: [GIT PULL] SafeSetID LSM changes for v5.8

[Micah Morton]

On Sun, Jun 14, 2020 at 12:20 PM Linus Torvalds
 wrote:
>
> On Sun, Jun 14, 2020 at 12:12 PM Micah Morton  wrote:
> >
> > That said I'm a little fuzzy on where to draw the line for which kinds
> > of changes really should be required to have bake time in -next. If
> > you think this is one of those cases, we can hold off on this until we
> > have some bake time for v5.9.
>
> It's merged, but in general the rule for "bake in -next" should be
> absolutely everything.
>
> The only exception is just pure and plain fixes.

Sounds good, that makes it pretty clear.

Thanks

>
> This SafeSetID change should in fact have been there for two different
> reasons: not only was it a new feature rather than a fix (in
> linux-next just for testing), it was one that crossed subsystem
> borders (should be in linux-next just for cross-subsystem testing). It
> touched files that very much aren't touched by just you.
>
> "Looks obvious" has nothing to do with avoiding linux-next.
>
> I suspect most of the bugs we have tend to be in code that "looked
> obvious" to somebody.
>
>  Linus

Re: [GIT PULL] SafeSetID LSM changes for v5.8

[Micah Morton]

On Sun, Jun 14, 2020 at 10:21 PM James Morris  wrote:
>
> On Sun, 14 Jun 2020, Micah Morton wrote:
>
> > This patch was sent to the security mailing list and there were no 
> > objections.
>
> Standard practice for new or modified LSM hooks is that they are reviewed
> and acked by maintainers of major LSMs (SELinux, AppArmor, and Smack, at
> least).
>
> "No objections" should be considered "not reviewed".
>
> Can you add your tree to linux-next?
> https://www.kernel.org/doc/man-pages/linux-next.html

Sure, I can do that. I should just send an email to Stephen Rothwell
asking him to include the -next branch from my tree?

>
> --
> James Morris
> 
>

Re: [GIT PULL] SafeSetID LSM changes for v5.8

[Micah Morton]

On Sun, Jun 14, 2020 at 11:39 AM Linus Torvalds
 wrote:
>
> On Sun, Jun 14, 2020 at 11:04 AM Micah Morton  wrote:
> >
> > I amended the author on the lone commit in this pull request. For some
> > reason I was thinking using the "From:" line in the commit body was
> > how I should make things show up as Thomas as the author and me as the
> > committer, but looks like that’s not true.
>
> That's how we do things in email, since you want a separate author for
> the emailed patch than from the author of the email itself.
>
> But git itself very much has that difference between "author" and
> "committer" internally, and all the usual email application tools will
> take the separate "From:" line from the email, and make that be the
> author in git.

Ah makes sense, thanks!

>
> (And then the sign-off chain is where we describe the whole path,
> because git only has the concept of those two end-points: the original
> author, and the final committer, but no concept of the path in between
> the two, nor does it have the concept of the copyright and license
> agreement implications of the sign-offs).
>
> > I also removed my own Signed-off-by line from the pull request body
> > and included it in the commit instead of the Reviewed-by line.
>
> Good. You will get credit for the pull request in the merge commit
> itself as a "Pull xyz from Micah Morton", so that path of history gets
> encoded that way.
>
> But the sign-off chain is supposed to be there for each individual commit.
>
> (I don't always notice those things, but afaik there is automation in
> place in -next that should warn about commits with incomplete sign-off
> chains. Did that not trigger for some reason in this case?).

This change hasn't had any bake time in linux-next. I didn't deem it
sufficiently risky or complex to warrant such integration testing.

That said I'm a little fuzzy on where to draw the line for which kinds
of changes really should be required to have bake time in -next. If
you think this is one of those cases, we can hold off on this until we
have some bake time for v5.9.

Either way this is a good reminder for the v5.9 merge window when we
plan to have more featureful changes going in for SafeSetID (although
those will be completely internal to the LSM, so low chance of
breaking anything outside the module).

>
>   Linus

Re: [GIT PULL] SafeSetID LSM changes for v5.8

[Micah Morton]

I amended the author on the lone commit in this pull request. For some
reason I was thinking using the "From:" line in the commit body was
how I should make things show up as Thomas as the author and me as the
committer, but looks like that’s not true.

I also removed my own Signed-off-by line from the pull request body
and included it in the commit instead of the Reviewed-by line.

Thanks,
Micah


The following changes since commit 3d77e6a8804abcc0504c904bd6e5cdf3a5cf8162:

  Linux 5.7 (2020-05-31 16:49:15 -0700)

are available in the Git repository at:

  https://github.com/micah-morton/linux.git
tags/LSM-add-setgid-hook-5.8-author-fix

for you to fetch changes up to 39030e1351aa1aa7443bb2da24426573077c83da:

  security: Add LSM hooks to set*gid syscalls (2020-06-14 10:52:02 -0700)


Add additional LSM hooks for SafeSetID

SafeSetID is capable of making allow/deny decisions for set*uid calls
on a system, and we want to add similar functionality for set*gid
calls. The work to do that is not yet complete, so probably won't make
it in for v5.8, but we are looking to get this simple patch in for
v5.8 since we have it ready. We are planning on the rest of the work
for extending the SafeSetID LSM being merged during the v5.9 merge
window.

This patch was sent to the security mailing list and there were no objections.


Thomas Cedeno (1):
  security: Add LSM hooks to set*gid syscalls

 include/linux/lsm_hook_defs.h |  2 ++
 include/linux/lsm_hooks.h |  9 +
 include/linux/security.h  |  9 +
 kernel/sys.c  | 15 ++-
 security/security.c   |  6 ++
 5 files changed, 40 insertions(+), 1 deletion(-)

On Fri, Jun 12, 2020 at 2:23 PM Linus Torvalds
 wrote:
>
> Finally emptied my normal pull request queue and starting to look at
> things I wanted to look at more closely..
>
> On Tue, Jun 9, 2020 at 11:26 AM Micah Morton  wrote:
> >
> > This patch was sent to the security mailing list and there were no 
> > objections.
>
> That patch as committed has both the wrong authorship, and the wrong
> sign-off chain.
>
> Not pulling.
>
>  Linus

[GIT PULL] SafeSetID LSM changes for v5.8

[Micah Morton]

The following changes since commit 3d77e6a8804abcc0504c904bd6e5cdf3a5cf8162:

  Linux 5.7 (2020-05-31 16:49:15 -0700)

are available in the Git repository at:

  https://github.com/micah-morton/linux.git tags/LSM-add-setgid-hook-5.8

for you to fetch changes up to 04d244bcf92f525011e3df34b21fc39b0591ba93:

  security: Add LSM hooks to set*gid syscalls (2020-06-09 10:22:13 -0700)


Add additional LSM hooks for SafeSetID

SafeSetID is capable of making allow/deny decisions for set*uid calls
on a system, and we want to add similar functionality for set*gid
calls. The work to do that is not yet complete, so probably won't make
it in for v5.8, but we are looking to get this simple patch in for
v5.8 since we have it ready. We are planning on the rest of the work
for extending the SafeSetID LSM being merged during the v5.9 merge
window.

This patch was sent to the security mailing list and there were no objections.

Signed-off-by: Micah Morton 


Micah Morton (1):
  security: Add LSM hooks to set*gid syscalls

 include/linux/lsm_hook_defs.h |  2 ++
 include/linux/lsm_hooks.h |  9 +
 include/linux/security.h  |  9 +
 kernel/sys.c  | 15 ++-
 security/security.c   |  6 ++
 5 files changed, 40 insertions(+), 1 deletion(-)

Re: [GIT PULL] SafeSetID LSM changes for 5.4

[Micah Morton]

On Mon, Sep 23, 2019 at 5:45 PM Linus Torvalds
 wrote:
>
> On Mon, Sep 23, 2019 at 4:35 PM James Morris  
> wrote:
> >
> > My understanding is that SafeSetID is shipping in ChromeOS -- this was
> > part of the rationale for merging it.
>
> Well, if even the developer didn't test it for two months, I don't
> think "it's in upstream" makes any sense or difference.
>
>  Linus

Yes, SafeSetID is shipping on Chrome OS, although I agree having that
bug in 5.3 without anyone noticing is bad. When Jann sent the last
round of patches for 5.3 he had tested the code and everything looked
good, although I unfortunately neglected to test it again after a
tweak to one of the patches, which of course broke stuff when the
patches ultimately went in.

Even though this is enabled in production for Chrome OS, none of the
Chrome OS devices are using version 5.3 yet, so it went unnoticed on
Chrome OS so far. In general the fact that a kernel feature is
shipping on Chrome OS isn't an up-to-date assurance that the feature
works in the most recent Linux release, as it would likely be months
(at least) from when a change makes it into the kernel until that
kernel release is ever run on a Chrome OS device (right now the most
recent kernel we ship on Chrome OS is 4.19, so I've had to backport
the SafeSetID stuff).

We've found this SafeSetID LSM to be pretty useful on Chrome OS, and
more use cases have popped up than we had in mind when writing it,
which suggests others would potentially find it useful as well. But I
understand for it to be useful to others it needs to be stable and
functional on every release. The best way I know of ensuring this is
for me to personally run the SafeSetID selftest (in
tools/testing/selftests/safesetid/) every release, regardless of
whether we make any changes to SafeSetID itself. Does this sound
sufficient or are there more formal guidelines/processes here that I'm
not aware of?

[GIT PULL] SafeSetID LSM changes for 5.4

[Micah Morton]

The following changes since commit 609488bc979f99f805f34e9a32c1e3b71179d10b:

  Linux 5.3-rc2 (2019-07-28 12:47:02 -0700)

are available in the Git repository at:

  https://github.com/micah-morton/linux.git tags/safesetid-bugfix-5.4

for you to fetch changes up to 21ab8580b383f27b7f59b84ac1699cb26d6c3d69:

  LSM: SafeSetID: Stop releasing uninitialized ruleset (2019-09-17
11:27:05 -0700)


Fix for SafeSetID bug that was introduced in 5.3

Jann Horn sent some patches to fix some bugs in SafeSetID for 5.3. After
he had done his testing there were a couple small code tweaks that went
in and caused this bug. From what I can see SafeSetID is broken in 5.3
and crashes the kernel every time during initialization if you try to
use it. I came across this bug when backporting Jann's changes for 5.3
to older kernels (4.14 and 4.19). I've tested on a Chrome OS device and
verified that this change fixes things. Unless I'm missing something it
doesn't seem super useful to have this change bake in linux-next, since it is
completely broken in 5.3 and nobody noticed.

Signed-off-by: Micah Morton 


Micah Morton (1):
  LSM: SafeSetID: Stop releasing uninitialized ruleset

 security/safesetid/securityfs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Re: [GIT PULL] SafeSetID MAINTAINERS file update for v5.3

[Micah Morton]

On Mon, Aug 5, 2019 at 12:27 PM Konstantin Ryabitsev
 wrote:
>
> On Mon, Aug 05, 2019 at 12:17:49PM -0700, Linus Torvalds wrote:
> >> However, I suspect that getting message-ids for all your pull
> >> requests
> >> would significantly complicate your workflow.
> >
> >Yeah, that would be a noticeable annoyance. If I were to process pull
> >requests the way I used to process emailed patches (ie "git am -s" on
> >a mailbox) that would be a natural thing to perhaps do, but it's not
> >at all how it ends up working. Having to save the pull request email
> >to then process it with some script would turn it into a chore.
> >
> >I think the pr-tracker-bot clearly catches most cases as it is, and
> >it's only the occasional "somebody did something odd" that then misses
> >an automated response. Not a huge deal. For me it was actually more
> >the "I didn't understand why the response didn't happen", not so much
> >"I really want to always see responses".
>
> Ok, let me add a fix for Re: at the start -- this won't make things
> significantly more expensive, but will catch this particular corner
> case.
>
> Best regards,
> -K

Linus, thanks for the tips earlier about gitk. I'll use that in the future.

Unfortunately I didn't have the mental model quite right of what
happens during the pull request. I was thinking along the lines of my
commits being cherry picked onto your tree, rather than how it
actually happens with git merge where my tree's commit history needs
to match yours perfectly.

Re: [GIT PULL] SafeSetID MAINTAINERS file update for v5.3

[Micah Morton]

Sorry about that. To fix it I did a "git reset hard" to before any of
those commits by Jann Horn, then fast-forwarded to the v5.3-rc2 tag
and force pushed that to my origin/master then pushed a new branch up
with my MAINTAINERS file changes. Hopefully this is a valid fix.

--
The following changes since commit 609488bc979f99f805f34e9a32c1e3b71179d10b:

  Linux 5.3-rc2 (2019-07-28 12:47:02 -0700)

are available in the Git repository at:

  https://github.com/micah-morton/linux.git
  tags/safesetid-maintainers-correction-5.3-rc2

for you to fetch changes up to fc5b34a35458314df1dd00281f6e41f419581aa9:

  Add entry in MAINTAINERS file for SafeSetID LSM (2019-08-01 10:30:57 -0700)


Add entry in MAINTAINERS file for SafeSetID LSM.

Has not had any bake time or testing, since its just changes to a text file.

--------
Micah Morton (1):
  Add entry in MAINTAINERS file for SafeSetID LSM

 MAINTAINERS | 6 ++
 1 file changed, 6 insertions(+)

On Thu, Aug 1, 2019 at 6:25 AM Linus Torvalds
 wrote:
>
> On Wed, Jul 31, 2019 at 2:30 PM Micah Morton  wrote:
> >
> > You mentioned a couple weeks ago it would be good if I added myself to
> > the MAINTAINERS file for the SafeSetID LSM. Here's the pull request
> > for v5.3.
>
> There's a lot more than the maintainer ID in there. You've rebased old
> patches that I already had etc:
>
>   Jann Horn (10):
>   LSM: SafeSetID: fix pr_warn() to include newline
>   LSM: SafeSetID: fix check for setresuid(new1, new2, new3)
>   LSM: SafeSetID: refactor policy hash table
>   LSM: SafeSetID: refactor safesetid_security_capable()
>   LSM: SafeSetID: refactor policy parsing
>   LSM: SafeSetID: fix userns handling in securityfs
>   LSM: SafeSetID: rewrite userspace API to atomic updates
>   LSM: SafeSetID: add read handler
>   LSM: SafeSetID: verify transitive constrainedness
>   LSM: SafeSetID: fix use of literal -1 in capable hook
>
>   Micah Morton (2):
>   Merge commit 'v5.3-rc2^0'
>   Add entry in MAINTAINERS file for SafeSetID LSM
>
> Not pulled.
>
>   Linus

[GIT PULL] SafeSetID MAINTAINERS file update for v5.3

[Micah Morton]

Hi Linus,

You mentioned a couple weeks ago it would be good if I added myself to
the MAINTAINERS file for the SafeSetID LSM. Here's the pull request
for v5.3.

Thanks!
--
The following changes since commit 179757afbef5f64b9bd25e6161f72fc1a52a8f2e:

  Merge commit 'v5.3-rc2^0' (2019-07-31 13:45:16 -0700)

are available in the Git repository at:

  https://github.com/micah-morton/linux.git tags/safesetid-maintainers-5.3-rc2

for you to fetch changes up to 7e20e910eabdf0af90fd10e712f15b413be8e135:

  Add entry in MAINTAINERS file for SafeSetID LSM (2019-07-31 13:58:11 -0700)


Add entry in MAINTAINERS file for SafeSetID LSM.

Has not had any bake time or testing, since its just changes to a text file.


Micah Morton (1):
  Add entry in MAINTAINERS file for SafeSetID LSM

 MAINTAINERS | 6 ++
 1 file changed, 6 insertions(+)

Re: [GIT PULL] SafeSetID LSM changes for 5.3

[Micah Morton]

On Tue, Jul 16, 2019 at 12:06 PM Linus Torvalds
 wrote:
>
> On Mon, Jul 15, 2019 at 9:05 AM Micah Morton  wrote:
> >
> > I'm maintaining the new SafeSetID LSM and was told to set up my own
> > tree for sending pull requests rather than sending my changes through
> > James Morris and the security subsystem tree.
>
> Yes. It would be good if you also added yourself to the MAINTAINERS
> file. Right now there's no entry for security/safesetid at all.

Yes, I have a patch for this but was told it would be better to send
the patch through my tree rather than the security tree. I can send a
pull request for that.

>
> > This is my first time doing one of these pull requests so hopefully I
> > didn't screw something up.
>
> So a couple of notes:
>
>  - *please* don't rebase your work in the day before

Got it.

>
>Was this in linux-next? was this tested at all? Hard to tell, since
> it was rebased recently, so for all I know it's all completely new

This was not in linux-next, but was tested by Jann on a Chrome OS
device. There's also the selftest for this code. But I can send
non-trivial stuff to linux-next first next time.

>
>  - don't use a random kernel-of-the-day as the base for development

Got it.

>
>This is related to the rebasing issue, but is true even if you
> don't rebase. There is no way that it was a good idea to pick my
> random - possibly completely broken - kernel from Sunday afternoon in
> the middle of a merge window as a base for development.
>
>If you start development, or if you have to rebase (for some *good*
> reason) you need to do so on a good stable base, not on the quick-sand
> that is "random kernel of the day during the busiest merge activity".

Makes sense. The development was not actually done on that kernel, I
just grabbed that random kernel for committing the changes on top of
(these changes were developed a little while ago, but they're all self
contained to the SafeSetID LSM), but I'll pick a stable one next time.

>
>  - Please use the "git pull-request" format and then add any extra
> notes you feel are necessary
>
>Yes, your pull request is *almost* git pull-request, but you seem
> to have actively removed whitespace making it almost illegible. It's
> really hard to pick out the line that has the actual git repository
> address, because it's basically hidden inside one big blob of text.
>
> I've pulled this as-is since it's the first time, but I expect better next 
> time.
>
> There are various resources on some cleanliness issues, and people
> fairly recently tried to combine it under
>
>Documentation/maintainer/rebasing-and-merging.rst
>
> which covers at least the basics on why not to rebase etc.

Thanks for the pointer. I had not seen that yet.

>
> And if you *do* end up rebasing, consider the end result "untested",
> so then it should have been done before the merge window even started,
> and the rebased branch should have been in linux-next. And not sent to
> me the very next day.

Yep, makes sense.

>
>Linus

Thanks!

[GIT PULL] SafeSetID LSM changes for 5.3

[Micah Morton]

Hi Linus,

I'm maintaining the new SafeSetID LSM and was told to set up my own
tree for sending pull requests rather than sending my changes through
James Morris and the security subsystem tree.

This is my first time doing one of these pull requests so hopefully I
didn't screw something up.

Thanks,
Micah
---
The following changes since commit fec88ab0af9706b2201e5daf377c5031c62d11f7:
Merge tag 'for-linus-hmm' of
git://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma (2019-07-14
19:42:11 -0700)
are available in the Git repository at:
https://github.com/micah-morton/linux.git tags/safesetid-5.3
for you to fetch changes up to e10337daefecb47209fd2af5f4fab0d1a370737f:
LSM: SafeSetID: fix use of literal -1 in capable hook (2019-07-15
08:08:03 -0700)

SafeSetID patches for 5.3
These changes from Jann Horn fix a couple issues in the recently added
SafeSetID LSM:

(1) There was a simple logic bug in one of the hooks for the LSM where
the code was incorrectly returning early in some cases before all
security checks had been passed.
(2) There was a more high level issue with how this LSM gets configured
that could allow for a program to bypass the security restrictions
by switching to an allowed UID and then again to any other UID on
the system if the target UID of the first transition is
unconstrained on the system. Luckily this is an easy fix that we now
enforce at the time the LSM gets configured.

There are also some changes from Jann that make policy updates for this
LSM atomic. Kees Cook, Jann and myself have reviewed these changes and they
look good from our point of view.
Signed-off-by: Micah Morton 

Jann Horn (10):
LSM: SafeSetID: fix pr_warn() to include newline
LSM: SafeSetID: fix check for setresuid(new1, new2, new3)
LSM: SafeSetID: refactor policy hash table
LSM: SafeSetID: refactor safesetid_security_capable()
LSM: SafeSetID: refactor policy parsing
LSM: SafeSetID: fix userns handling in securityfs
LSM: SafeSetID: rewrite userspace API to atomic updates
LSM: SafeSetID: add read handler
LSM: SafeSetID: verify transitive constrainedness
LSM: SafeSetID: fix use of literal -1 in capable hook
security/safesetid/lsm.c | 276 +-
security/safesetid/lsm.h | 34 --
security/safesetid/securityfs.c | 307
+--
tools/testing/selftests/safesetid/safesetid-test.c | 18 ++-
4 files changed, 306 insertions(+), 329 deletions(-)

Re: WARNING in apparmor_secid_to_secctx

[Micah Morton]

On Wed, Jan 30, 2019 at 6:45 AM Dmitry Vyukov  wrote:
>
> On Tue, Jan 29, 2019 at 12:32 PM Tetsuo Handa
>  wrote:
> >
> > On 2018/09/06 19:59, Dmitry Vyukov wrote:
> > > On Wed, Sep 5, 2018 at 7:37 PM, Casey Schaufler  
> > > wrote:
> > >> On 9/5/2018 4:08 AM, Dmitry Vyukov wrote:
> > >>> Thanks! I've re-enabled selinux on syzbot:
> > >>> https://github.com/google/syzkaller/commit/196410e4f5665d4d2bf6c818d06f1c8d03cfa8cc
> > >>> Now we will have instances with apparmor and with selinux.
> > >>
> > >> Any chance we could get a Smack instance as well?
> > >
> > > Hi Casey,
> > >
> > > Sure!
> > > Provided you want to fix bugs ;)
> > > I've setup an instance with smack enabled:
> > > https://github.com/google/syzkaller/commit/0bb7a7eb8e0958c6fbe2d69615b9fae4af88c8ee
> > >
> >
> > Dmitry, is it possible to update configs for linux-next.git , for
> > we want to test a big change in LSM which will go to Linux 5.1 ?
> >
> > TOMOYO security module (CONFIG_SECURITY_TOMOYO=y) can now coexist with
> > SELinux/Smack/AppArmor security modules, and SafeSetID security module
> > (CONFIG_SECURITY_SAFESETID=y) was added. Testing with these modules also
> > enabled might find something...
>
> Hi,
>
> syzbot configs/cmdline args are stored here:
> https://github.com/google/syzkaller/tree/master/dashboard/config
>
> I've tried to update to the latest kernel, the diff is below.
> Few questions:
> 1. How are modules enabled now?
> We pass security=selinux of security=smack on command line. What do we
> need to pass now to enable several modules at the same time?
>
> 2. What exactly does SECURITY_SAFESETID do?
> syzkaller executor uses setuid/gid, and the config says "to only those
> approved by a system-wide whitelist". What/where is that whitelist? If
> we just enable it, but not whitelist will it break syzkaller?

There is a description of this recently-added LSM in
Documentation/admin-guide/LSM/SafeSetID.rst (has been pulled into
next-general branch in the linux-security tree, should be in
linux-next tree by now as well). TL;DR: if no whitelist is configured
by writing to the safesetid directory in securityfs, there will be no
policies preventing certain users from changing to other users/groups.
So shouldn't break syzkaller if you "just" enable it.


>
>
>
> diff --git a/dashboard/config/upstream-kasan.config
> b/dashboard/config/upstream-kasan.config
> index 475d84a3..ed026cd8 100644
> --- a/dashboard/config/upstream-kasan.config
> +++ b/dashboard/config/upstream-kasan.config
> @@ -1,6 +1,6 @@
>  #
>  # Automatically generated file; DO NOT EDIT.
> -# Linux/x86 4.20.0 Kernel Configuration
> +# Linux/x86 5.0.0-rc4 Kernel Configuration
>  #
>
>  # The following configs are added manually, preserve them.
> @@ -14,11 +14,12 @@ CONFIG_DEBUG_MEMORY=y
>  CONFIG_DEBUG_AID_FOR_SYZBOT=y
>
>  #
> -# Compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> +# Compiler:
>  #
>  CONFIG_CC_IS_GCC=y
>  CONFIG_GCC_VERSION=9
>  CONFIG_CLANG_VERSION=0
> +CONFIG_CC_HAS_ASM_GOTO=y
>  CONFIG_CONSTRUCTORS=y
>  CONFIG_IRQ_WORK=y
>  CONFIG_BUILDTIME_EXTABLE_SORT=y
> @@ -295,7 +296,7 @@ CONFIG_X86_X2APIC=y
>  CONFIG_X86_MPPARSE=y
>  # CONFIG_GOLDFISH is not set
>  CONFIG_RETPOLINE=y
> -# CONFIG_RESCTRL is not set
> +# CONFIG_X86_RESCTRL is not set
>  CONFIG_X86_EXTENDED_PLATFORM=y
>  # CONFIG_X86_NUMACHIP is not set
>  # CONFIG_X86_VSMP is not set
> @@ -571,6 +572,7 @@ CONFIG_X86_ACPI_CPUFREQ_CPB=y
>  CONFIG_CPU_IDLE=y
>  # CONFIG_CPU_IDLE_GOV_LADDER is not set
>  CONFIG_CPU_IDLE_GOV_MENU=y
> +# CONFIG_CPU_IDLE_GOV_TEO is not set
>  CONFIG_INTEL_IDLE=y
>
>  #
> @@ -745,8 +747,9 @@ CONFIG_HAVE_ARCH_PREL32_RELOCATIONS=y
>  #
>  # CONFIG_GCOV_KERNEL is not set
>  CONFIG_ARCH_HAS_GCOV_PROFILE_ALL=y
> -CONFIG_PLUGIN_HOSTCC=""
> +CONFIG_PLUGIN_HOSTCC="g++"
>  CONFIG_HAVE_GCC_PLUGINS=y
> +# CONFIG_GCC_PLUGINS is not set
>  CONFIG_RT_MUTEXES=y
>  CONFIG_BASE_SMALL=0
>  CONFIG_MODULES=y
> @@ -932,6 +935,7 @@ CONFIG_NET_KEY_MIGRATE=y
>  CONFIG_SMC=y
>  CONFIG_SMC_DIAG=y
>  CONFIG_XDP_SOCKETS=y
> +CONFIG_XDP_SOCKETS_DIAG=y
>  CONFIG_INET=y
>  CONFIG_IP_MULTICAST=y
>  CONFIG_IP_ADVANCED_ROUTER=y
> @@ -3246,6 +3250,7 @@ CONFIG_SPI_MASTER=y
>  CONFIG_SPI_BITBANG=y
>  # CONFIG_SPI_CADENCE is not set
>  # CONFIG_SPI_DESIGNWARE is not set
> +# CONFIG_SPI_NXP_FLEXSPI is not set
>  CONFIG_SPI_PXA2XX=y
>  CONFIG_SPI_PXA2XX_PCI=y
>  # CONFIG_SPI_ROCKCHIP is not set
> @@ -3896,6 +3901,10 @@ CONFIG_DRM_KMS_CMA_HELPER=y
>  # CONFIG_DRM_I2C_SIL164 is not set
>  # CONFIG_DRM_I2C_NXP_TDA998X is not set
>  # CONFIG_DRM_I2C_NXP_TDA9950 is not set
> +
> +#
> +# ARM devices
> +#
>  # CONFIG_DRM_RADEON is not set
>  # CONFIG_DRM_AMDGPU is not set
>
> @@ -3951,6 +3960,7 @@ CONFIG_DRM_PANEL_BRIDGE=y
>  # Display Interface Bridges
>  #
>  # CONFIG_DRM_ANALOGIX_ANX78XX is not set
> +# CONFIG_DRM_ETNAVIV is not set
>  # CONFIG_DRM_HISI_HIBMC is not set
>  CONFIG_DRM_TINYDRM=y
>  # CONFIG_TINYDRM_HX8357D is not set
> @@ -4060,7 +4070,6 @@ CONFIG_FRAMEBUFFER_CONSOLE_DETECT_PRIMARY=y
>  # 

Re: linux-next: Tree for Jan 29 (security/safesetid/)

[Micah Morton]

I noticed you don't have the following lines (or some of the other
related security ones) in your .config.

CONFIG_SECURITY=y
CONFIG_SECURITY_WRITABLE_HOOKS=y

Seems like we need a 'depends on SECURITY' line (or something like
that) in security/safesetid/Kconfig -- let me see if that fixes things
and if so I'll upload a patch

On Tue, Jan 29, 2019 at 9:04 AM Randy Dunlap  wrote:
>
> On 1/28/19 10:11 PM, Stephen Rothwell wrote:
> > Hi all,
> >
> > Changes since 20190125:
> >
>
> on x86_64:
>
> ld: security/safesetid/lsm.o:(.data+0x10): undefined reference to 
> `security_hook_heads'
> ld: security/safesetid/lsm.o:(.data+0x38): undefined reference to 
> `security_hook_heads'
> ld: security/safesetid/lsm.o: in function `safesetid_security_init':
> lsm.c:(.init.text+0x14): undefined reference to `security_add_hooks'
>
>
> Full randconfig file is attached.
>
> --
> ~Randy

[PATCH] security: Add LSM fixup hooks to set*gid syscalls.

[Micah Morton]

The set*uid system calls all call an LSM fixup hook called
security_task_fix_setuid, which allows for altering the behavior of those
calls by a security module. Comments explaining the LSM_SETID_* constants
in /include/linux/security.h imply that the constants are to be used for
both the set*uid and set*gid syscalls, but the set*gid syscalls do not
have the relevant hooks, meaning a security module can only alter syscalls
that change user identity attributes but not ones that change group
identity attributes. This patch adds the necessary LSM hook, called
security_task_fix_setgid, and calls the hook from the appropriate places
in the set*gid syscalls.Tested by putting a print statement in the hook and
seeing it triggered from the various set*gid syscalls.

Signed-off-by: Micah Morton 
Acked-by: Kees Cook 
---
NOTE: the security_task_fix_setgid line in sys_setfsgid is over 80
characters, but I figured I'd just follow how it was done in sys_setfsuid
rather than trying to wrap the line, since the functions are nearly
identical.
---
 include/linux/lsm_hooks.h | 12 
 include/linux/security.h  |  9 +
 kernel/sys.c  | 15 ++-
 security/security.c   |  6 ++
 4 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 8f1131c8dd54..a2166c812a97 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -599,6 +599,15 @@
  * @old is the set of credentials that are being replaces
  * @flags contains one of the LSM_SETID_* values.
  * Return 0 on success.
+ * @task_fix_setgid:
+ * Update the module's state after setting one or more of the group
+ * identity attributes of the current process.  The @flags parameter
+ * indicates which of the set*gid system calls invoked this hook.
+ * @new is the set of credentials that will be installed.  Modifications
+ * should be made to this rather than to @current->cred.
+ * @old is the set of credentials that are being replaced
+ * @flags contains one of the LSM_SETID_* values.
+ * Return 0 on success.
  * @task_setpgid:
  * Check permission before setting the process group identifier of the
  * process @p to @pgid.
@@ -1587,6 +1596,8 @@ union security_list_options {
 enum kernel_read_file_id id);
int (*task_fix_setuid)(struct cred *new, const struct cred *old,
int flags);
+   int (*task_fix_setgid)(struct cred *new, const struct cred *old,
+   int flags);
int (*task_setpgid)(struct task_struct *p, pid_t pgid);
int (*task_getpgid)(struct task_struct *p);
int (*task_getsid)(struct task_struct *p);
@@ -1876,6 +1887,7 @@ struct security_hook_heads {
struct hlist_head kernel_post_read_file;
struct hlist_head kernel_module_request;
struct hlist_head task_fix_setuid;
+   struct hlist_head task_fix_setgid;
struct hlist_head task_setpgid;
struct hlist_head task_getpgid;
struct hlist_head task_getsid;
diff --git a/include/linux/security.h b/include/linux/security.h
index 63030c85ee19..a82d97cf13ab 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -325,6 +325,8 @@ int security_kernel_post_read_file(struct file *file, char 
*buf, loff_t size,
   enum kernel_read_file_id id);
 int security_task_fix_setuid(struct cred *new, const struct cred *old,
 int flags);
+int security_task_fix_setgid(struct cred *new, const struct cred *old,
+int flags);
 int security_task_setpgid(struct task_struct *p, pid_t pgid);
 int security_task_getpgid(struct task_struct *p);
 int security_task_getsid(struct task_struct *p);
@@ -929,6 +931,13 @@ static inline int security_task_fix_setuid(struct cred 
*new,
return cap_task_fix_setuid(new, old, flags);
 }
 
+static inline int security_task_fix_setgid(struct cred *new,
+  const struct cred *old,
+  int flags)
+{
+   return 0;
+}
+
 static inline int security_task_setpgid(struct task_struct *p, pid_t pgid)
 {
return 0;
diff --git a/kernel/sys.c b/kernel/sys.c
index 38509dc1f77b..f6ef922c6815 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -392,6 +392,10 @@ long __sys_setregid(gid_t rgid, gid_t egid)
new->sgid = new->egid;
new->fsgid = new->egid;
 
+   retval = security_task_fix_setgid(new, old, LSM_SETID_RE);
+   if (retval < 0)
+   goto error;
+
return commit_creds(new);
 
 error:
@@ -434,6 +438,10 @@ long __sys_setgid(gid_t gid)
else
goto error;
 
+   retval = security_task_fix_setgid(new, old, LSM_SETID_ID);
+   if (retval < 0)
+   goto error;
+
return commit_creds(new)

[PATCH] security: Add LSM fixup hooks to set*gid syscalls.

[Micah Morton]

The set*uid system calls all call an LSM fixup hook called
security_task_fix_setuid, which allows for altering the behavior of those
calls by a security module. Comments explaining the LSM_SETID_* constants
in /include/linux/security.h imply that the constants are to be used for
both the set*uid and set*gid syscalls, but the set*gid syscalls do not
have the relevant hooks, meaning a security module can only alter syscalls
that change user identity attributes but not ones that change group
identity attributes. This patch adds the necessary LSM hook, called
security_task_fix_setgid, and calls the hook from the appropriate places
in the set*gid syscalls.Tested by putting a print statement in the hook and
seeing it triggered from the various set*gid syscalls.

Signed-off-by: Micah Morton 
Acked-by: Kees Cook 
---
NOTE: the security_task_fix_setgid line in sys_setfsgid is over 80
characters, but I figured I'd just follow how it was done in sys_setfsuid
rather than trying to wrap the line, since the functions are nearly
identical.
---
 include/linux/lsm_hooks.h | 12 
 include/linux/security.h  |  9 +
 kernel/sys.c  | 15 ++-
 security/security.c   |  6 ++
 4 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 8f1131c8dd54..a2166c812a97 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -599,6 +599,15 @@
  * @old is the set of credentials that are being replaces
  * @flags contains one of the LSM_SETID_* values.
  * Return 0 on success.
+ * @task_fix_setgid:
+ * Update the module's state after setting one or more of the group
+ * identity attributes of the current process.  The @flags parameter
+ * indicates which of the set*gid system calls invoked this hook.
+ * @new is the set of credentials that will be installed.  Modifications
+ * should be made to this rather than to @current->cred.
+ * @old is the set of credentials that are being replaced
+ * @flags contains one of the LSM_SETID_* values.
+ * Return 0 on success.
  * @task_setpgid:
  * Check permission before setting the process group identifier of the
  * process @p to @pgid.
@@ -1587,6 +1596,8 @@ union security_list_options {
 enum kernel_read_file_id id);
int (*task_fix_setuid)(struct cred *new, const struct cred *old,
int flags);
+   int (*task_fix_setgid)(struct cred *new, const struct cred *old,
+   int flags);
int (*task_setpgid)(struct task_struct *p, pid_t pgid);
int (*task_getpgid)(struct task_struct *p);
int (*task_getsid)(struct task_struct *p);
@@ -1876,6 +1887,7 @@ struct security_hook_heads {
struct hlist_head kernel_post_read_file;
struct hlist_head kernel_module_request;
struct hlist_head task_fix_setuid;
+   struct hlist_head task_fix_setgid;
struct hlist_head task_setpgid;
struct hlist_head task_getpgid;
struct hlist_head task_getsid;
diff --git a/include/linux/security.h b/include/linux/security.h
index 63030c85ee19..a82d97cf13ab 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -325,6 +325,8 @@ int security_kernel_post_read_file(struct file *file, char 
*buf, loff_t size,
   enum kernel_read_file_id id);
 int security_task_fix_setuid(struct cred *new, const struct cred *old,
 int flags);
+int security_task_fix_setgid(struct cred *new, const struct cred *old,
+int flags);
 int security_task_setpgid(struct task_struct *p, pid_t pgid);
 int security_task_getpgid(struct task_struct *p);
 int security_task_getsid(struct task_struct *p);
@@ -929,6 +931,13 @@ static inline int security_task_fix_setuid(struct cred 
*new,
return cap_task_fix_setuid(new, old, flags);
 }
 
+static inline int security_task_fix_setgid(struct cred *new,
+  const struct cred *old,
+  int flags)
+{
+   return 0;
+}
+
 static inline int security_task_setpgid(struct task_struct *p, pid_t pgid)
 {
return 0;
diff --git a/kernel/sys.c b/kernel/sys.c
index 38509dc1f77b..f6ef922c6815 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -392,6 +392,10 @@ long __sys_setregid(gid_t rgid, gid_t egid)
new->sgid = new->egid;
new->fsgid = new->egid;
 
+   retval = security_task_fix_setgid(new, old, LSM_SETID_RE);
+   if (retval < 0)
+   goto error;
+
return commit_creds(new);
 
 error:
@@ -434,6 +438,10 @@ long __sys_setgid(gid_t gid)
else
goto error;
 
+   retval = security_task_fix_setgid(new, old, LSM_SETID_ID);
+   if (retval < 0)
+   goto error;
+
return commit_creds(new)

Re: [PATCH v2] security: Add LSM fixup hooks to set*gid syscalls.

[Micah Morton]

The ChromiumOS LSM used by ChromeOS will provide a hook for this, in
order to enforce ChromeOS-specific policies regarding which UIDs/GIDs a
process with CAP_SET{UID/GID} can transition to. The
security_task_fix_setuid LSM hook is very helpful in enabling such a feature
for ChromeOS that governs UID transitions, but unfortunately for us it looks
like the equivalent hook that would allow us to govern GID transitions from an
LSM never got added.

Resending with plain text mode enabled.

---

The set*uid system calls all call an LSM fixup hook called
security_task_fix_setuid, which allows for altering the behavior of those
calls by a security module. Comments explaining the LSM_SETID_* constants
in /include/linux/security.h imply that the constants are to be used for
both the set*uid and set*gid syscalls, but the set*gid syscalls do not
have the relevant hooks, meaning a security module can only alter syscalls
that change user identity attributes but not ones that change group
identity attributes. This patch adds the necessary LSM hook, called
security_task_fix_setgid, and calls the hook from the appropriate places
in the set*gid syscalls.Tested by putting a print statement in the hook and
seeing it triggered from the various set*gid syscalls.

Signed-off-by: Micah Morton 
Acked-by: Kees Cook 
---
NOTE: the security_task_fix_setgid line in sys_setfsgid is over 80
characters, but I figured I'd just follow how it was done in sys_setfsuid
rather than trying to wrap the line, since the functions are nearly
identical.

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 8f1131c8dd54..a2166c812a97 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -599,6 +599,15 @@
  * @old is the set of credentials that are being replaces
  * @flags contains one of the LSM_SETID_* values.
  * Return 0 on success.
+ * @task_fix_setgid:
+ * Update the module's state after setting one or more of the group
+ * identity attributes of the current process.  The @flags parameter
+ * indicates which of the set*gid system calls invoked this hook.
+ * @new is the set of credentials that will be installed.  Modifications
+ * should be made to this rather than to @current->cred.
+ * @old is the set of credentials that are being replaced
+ * @flags contains one of the LSM_SETID_* values.
+ * Return 0 on success.
  * @task_setpgid:
  * Check permission before setting the process group identifier of the
  * process @p to @pgid.
@@ -1587,6 +1596,8 @@ union security_list_options {
   enum kernel_read_file_id id);
  int (*task_fix_setuid)(struct cred *new, const struct cred *old,
  int flags);
+ int (*task_fix_setgid)(struct cred *new, const struct cred *old,
+ int flags);
  int (*task_setpgid)(struct task_struct *p, pid_t pgid);
  int (*task_getpgid)(struct task_struct *p);
  int (*task_getsid)(struct task_struct *p);
@@ -1876,6 +1887,7 @@ struct security_hook_heads {
  struct hlist_head kernel_post_read_file;
  struct hlist_head kernel_module_request;
  struct hlist_head task_fix_setuid;
+ struct hlist_head task_fix_setgid;
  struct hlist_head task_setpgid;
  struct hlist_head task_getpgid;
  struct hlist_head task_getsid;
diff --git a/include/linux/security.h b/include/linux/security.h
index 63030c85ee19..a82d97cf13ab 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -325,6 +325,8 @@ int security_kernel_post_read_file(struct file
*file, char *buf, loff_t size,
 enum kernel_read_file_id id);
 int security_task_fix_setuid(struct cred *new, const struct cred *old,
   int flags);
+int security_task_fix_setgid(struct cred *new, const struct cred *old,
+  int flags);
 int security_task_setpgid(struct task_struct *p, pid_t pgid);
 int security_task_getpgid(struct task_struct *p);
 int security_task_getsid(struct task_struct *p);
@@ -929,6 +931,13 @@ static inline int security_task_fix_setuid(struct
cred *new,
  return cap_task_fix_setuid(new, old, flags);
 }

+static inline int security_task_fix_setgid(struct cred *new,
+const struct cred *old,
+int flags)
+{
+ return 0;
+}
+
 static inline int security_task_setpgid(struct task_struct *p, pid_t pgid)
 {
  return 0;
diff --git a/kernel/sys.c b/kernel/sys.c
index 38509dc1f77b..f6ef922c6815 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -392,6 +392,10 @@ long __sys_setregid(gid_t rgid, gid_t egid)
  new->sgid = new->egid;
  new->fsgid = new->egid;

+ retval = security_task_fix_setgid(new, old, LSM_SETID_RE);
+ if (retval < 0)
+ goto error;
+
  return commit_creds(new);

 error:
@@ -434,6 +438,10 @@ long __sys_setgid(gid_t gid)
  else
  goto error;

+ retval = security_task_fix_setgid(new, old, LSM_SETID_ID);
+ if (retval < 0)
+ goto error;
+
  return commit_creds(new);

 error:
@@ -755,6 +763,10 @@ long __sys_setresgid(gid_t rgid, gid_t egid, gid_t sgid)
  new->sgid = ksgid;
  new->fsgid = new->egid;

+ retval = security_task_fix_setgid(new, old, LS

Re: [PATCH v2] security: Add LSM fixup hooks to set*gid syscalls.

[Micah Morton]

The ChromiumOS LSM used by ChromeOS will provide a hook for this, in
order to enforce ChromeOS-specific policies regarding which UIDs/GIDs a
process with CAP_SET{UID/GID} can transition to. The
security_task_fix_setuid LSM hook is very helpful in enabling such a feature
for ChromeOS that governs UID transitions, but unfortunately for us it looks
like the equivalent hook that would allow us to govern GID transitions from an
LSM never got added.

Resending with plain text mode enabled.

---

The set*uid system calls all call an LSM fixup hook called
security_task_fix_setuid, which allows for altering the behavior of those
calls by a security module. Comments explaining the LSM_SETID_* constants
in /include/linux/security.h imply that the constants are to be used for
both the set*uid and set*gid syscalls, but the set*gid syscalls do not
have the relevant hooks, meaning a security module can only alter syscalls
that change user identity attributes but not ones that change group
identity attributes. This patch adds the necessary LSM hook, called
security_task_fix_setgid, and calls the hook from the appropriate places
in the set*gid syscalls.Tested by putting a print statement in the hook and
seeing it triggered from the various set*gid syscalls.

Signed-off-by: Micah Morton 
Acked-by: Kees Cook 
---
NOTE: the security_task_fix_setgid line in sys_setfsgid is over 80
characters, but I figured I'd just follow how it was done in sys_setfsuid
rather than trying to wrap the line, since the functions are nearly
identical.

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 8f1131c8dd54..a2166c812a97 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -599,6 +599,15 @@
  * @old is the set of credentials that are being replaces
  * @flags contains one of the LSM_SETID_* values.
  * Return 0 on success.
+ * @task_fix_setgid:
+ * Update the module's state after setting one or more of the group
+ * identity attributes of the current process.  The @flags parameter
+ * indicates which of the set*gid system calls invoked this hook.
+ * @new is the set of credentials that will be installed.  Modifications
+ * should be made to this rather than to @current->cred.
+ * @old is the set of credentials that are being replaced
+ * @flags contains one of the LSM_SETID_* values.
+ * Return 0 on success.
  * @task_setpgid:
  * Check permission before setting the process group identifier of the
  * process @p to @pgid.
@@ -1587,6 +1596,8 @@ union security_list_options {
   enum kernel_read_file_id id);
  int (*task_fix_setuid)(struct cred *new, const struct cred *old,
  int flags);
+ int (*task_fix_setgid)(struct cred *new, const struct cred *old,
+ int flags);
  int (*task_setpgid)(struct task_struct *p, pid_t pgid);
  int (*task_getpgid)(struct task_struct *p);
  int (*task_getsid)(struct task_struct *p);
@@ -1876,6 +1887,7 @@ struct security_hook_heads {
  struct hlist_head kernel_post_read_file;
  struct hlist_head kernel_module_request;
  struct hlist_head task_fix_setuid;
+ struct hlist_head task_fix_setgid;
  struct hlist_head task_setpgid;
  struct hlist_head task_getpgid;
  struct hlist_head task_getsid;
diff --git a/include/linux/security.h b/include/linux/security.h
index 63030c85ee19..a82d97cf13ab 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -325,6 +325,8 @@ int security_kernel_post_read_file(struct file
*file, char *buf, loff_t size,
 enum kernel_read_file_id id);
 int security_task_fix_setuid(struct cred *new, const struct cred *old,
   int flags);
+int security_task_fix_setgid(struct cred *new, const struct cred *old,
+  int flags);
 int security_task_setpgid(struct task_struct *p, pid_t pgid);
 int security_task_getpgid(struct task_struct *p);
 int security_task_getsid(struct task_struct *p);
@@ -929,6 +931,13 @@ static inline int security_task_fix_setuid(struct
cred *new,
  return cap_task_fix_setuid(new, old, flags);
 }

+static inline int security_task_fix_setgid(struct cred *new,
+const struct cred *old,
+int flags)
+{
+ return 0;
+}
+
 static inline int security_task_setpgid(struct task_struct *p, pid_t pgid)
 {
  return 0;
diff --git a/kernel/sys.c b/kernel/sys.c
index 38509dc1f77b..f6ef922c6815 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -392,6 +392,10 @@ long __sys_setregid(gid_t rgid, gid_t egid)
  new->sgid = new->egid;
  new->fsgid = new->egid;

+ retval = security_task_fix_setgid(new, old, LSM_SETID_RE);
+ if (retval < 0)
+ goto error;
+
  return commit_creds(new);

 error:
@@ -434,6 +438,10 @@ long __sys_setgid(gid_t gid)
  else
  goto error;

+ retval = security_task_fix_setgid(new, old, LSM_SETID_ID);
+ if (retval < 0)
+ goto error;
+
  return commit_creds(new);

 error:
@@ -755,6 +763,10 @@ long __sys_setresgid(gid_t rgid, gid_t egid, gid_t sgid)
  new->sgid = ksgid;
  new->fsgid = new->egid;

+ retval = security_task_fix_setgid(new, old, LS

[PATCH v2] security: Add LSM fixup hooks to set*gid syscalls.

[Micah Morton]

The set*uid system calls all call an LSM fixup hook called
security_task_fix_setuid, which allows for altering the behavior of those
calls by a security module. Comments explaining the LSM_SETID_* constants
in /include/linux/security.h imply that the constants are to be used for
both the set*uid and set*gid syscalls, but the set*gid syscalls do not
have the relevant hooks, meaning a security module can only alter syscalls
that change user identity attributes but not ones that change group
identity attributes. This patch adds the necessary LSM hook, called
security_task_fix_setgid, and calls the hook from the appropriate places
in the set*gid syscalls.Tested by putting a print statement in the hook and
seeing it triggered from the various set*gid syscalls.

Signed-off-by: Micah Morton 
Acked-by: Kees Cook 
---
NOTE: the security_task_fix_setgid line in sys_setfsgid is over 80
characters, but I figured I'd just follow how it was done in sys_setfsuid
rather than trying to wrap the line, since the functions are nearly
identical.

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 8f1131c8dd54..a2166c812a97 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -599,6 +599,15 @@
  * @old is the set of credentials that are being replaces
  * @flags contains one of the LSM_SETID_* values.
  * Return 0 on success.
+ * @task_fix_setgid:
+ * Update the module's state after setting one or more of the group
+ * identity attributes of the current process.  The @flags parameter
+ * indicates which of the set*gid system calls invoked this hook.
+ * @new is the set of credentials that will be installed.
Modifications
+ * should be made to this rather than to @current->cred.
+ * @old is the set of credentials that are being replaced
+ * @flags contains one of the LSM_SETID_* values.
+ * Return 0 on success.
  * @task_setpgid:
  * Check permission before setting the process group identifier of the
  * process @p to @pgid.
@@ -1587,6 +1596,8 @@ union security_list_options {
   enum kernel_read_file_id id);
  int (*task_fix_setuid)(struct cred *new, const struct cred *old,
  int flags);
+ int (*task_fix_setgid)(struct cred *new, const struct cred *old,
+ int flags);
  int (*task_setpgid)(struct task_struct *p, pid_t pgid);
  int (*task_getpgid)(struct task_struct *p);
  int (*task_getsid)(struct task_struct *p);
@@ -1876,6 +1887,7 @@ struct security_hook_heads {
  struct hlist_head kernel_post_read_file;
  struct hlist_head kernel_module_request;
  struct hlist_head task_fix_setuid;
+ struct hlist_head task_fix_setgid;
  struct hlist_head task_setpgid;
  struct hlist_head task_getpgid;
  struct hlist_head task_getsid;
diff --git a/include/linux/security.h b/include/linux/security.h
index 63030c85ee19..a82d97cf13ab 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -325,6 +325,8 @@ int security_kernel_post_read_file(struct file *file,
char *buf, loff_t size,
 enum kernel_read_file_id id);
 int security_task_fix_setuid(struct cred *new, const struct cred *old,
   int flags);
+int security_task_fix_setgid(struct cred *new, const struct cred *old,
+  int flags);
 int security_task_setpgid(struct task_struct *p, pid_t pgid);
 int security_task_getpgid(struct task_struct *p);
 int security_task_getsid(struct task_struct *p);
@@ -929,6 +931,13 @@ static inline int security_task_fix_setuid(struct cred
*new,
  return cap_task_fix_setuid(new, old, flags);
 }

+static inline int security_task_fix_setgid(struct cred *new,
+const struct cred *old,
+int flags)
+{
+ return 0;
+}
+
 static inline int security_task_setpgid(struct task_struct *p, pid_t pgid)
 {
  return 0;
diff --git a/kernel/sys.c b/kernel/sys.c
index 38509dc1f77b..f6ef922c6815 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -392,6 +392,10 @@ long __sys_setregid(gid_t rgid, gid_t egid)
  new->sgid = new->egid;
  new->fsgid = new->egid;

+ retval = security_task_fix_setgid(new, old, LSM_SETID_RE);
+ if (retval < 0)
+ goto error;
+
  return commit_creds(new);

 error:
@@ -434,6 +438,10 @@ long __sys_setgid(gid_t gid)
  else
  goto error;

+ retval = security_task_fix_setgid(new, old, LSM_SETID_ID);
+ if (retval < 0)
+ goto error;
+
  return commit_creds(new);

 error:
@@ -755,6 +763,10 @@ long __sys_setresgid(gid_t rgid, gid_t egid, gid_t
sgid)
  new->sgid = ksgid;
  new->fsgid = new->egid;

+ retval = security_task_fix_setgid(new, old, LSM_SETID_RES);
+ if (retval < 0)
+ goto error;
+
  return commit_creds(new);

 error:
@@ -861,7 +873,8 @@ long __sys_setfsgid(gid_t gid)
  ns_capable(old->user_ns, CAP_SETGID)) {
  if (!gid_eq(kgid, old->fsgid)) {
  new->fsgid = kgid;
- goto change_okay;
+ if (security_task_fix_setgid(new, old, LSM_SETID_FS) == 0)
+ goto change_okay;
  }
  }

diff --git a/security/security.c b/security/security.c
index 68f46d849abe..587786fc0aaa 100644
--- a/security/security.c
+++ b/secu

[PATCH v2] security: Add LSM fixup hooks to set*gid syscalls.

[Micah Morton]

The set*uid system calls all call an LSM fixup hook called
security_task_fix_setuid, which allows for altering the behavior of those
calls by a security module. Comments explaining the LSM_SETID_* constants
in /include/linux/security.h imply that the constants are to be used for
both the set*uid and set*gid syscalls, but the set*gid syscalls do not
have the relevant hooks, meaning a security module can only alter syscalls
that change user identity attributes but not ones that change group
identity attributes. This patch adds the necessary LSM hook, called
security_task_fix_setgid, and calls the hook from the appropriate places
in the set*gid syscalls.Tested by putting a print statement in the hook and
seeing it triggered from the various set*gid syscalls.

Signed-off-by: Micah Morton 
Acked-by: Kees Cook 
---
NOTE: the security_task_fix_setgid line in sys_setfsgid is over 80
characters, but I figured I'd just follow how it was done in sys_setfsuid
rather than trying to wrap the line, since the functions are nearly
identical.

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 8f1131c8dd54..a2166c812a97 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -599,6 +599,15 @@
  * @old is the set of credentials that are being replaces
  * @flags contains one of the LSM_SETID_* values.
  * Return 0 on success.
+ * @task_fix_setgid:
+ * Update the module's state after setting one or more of the group
+ * identity attributes of the current process.  The @flags parameter
+ * indicates which of the set*gid system calls invoked this hook.
+ * @new is the set of credentials that will be installed.
Modifications
+ * should be made to this rather than to @current->cred.
+ * @old is the set of credentials that are being replaced
+ * @flags contains one of the LSM_SETID_* values.
+ * Return 0 on success.
  * @task_setpgid:
  * Check permission before setting the process group identifier of the
  * process @p to @pgid.
@@ -1587,6 +1596,8 @@ union security_list_options {
   enum kernel_read_file_id id);
  int (*task_fix_setuid)(struct cred *new, const struct cred *old,
  int flags);
+ int (*task_fix_setgid)(struct cred *new, const struct cred *old,
+ int flags);
  int (*task_setpgid)(struct task_struct *p, pid_t pgid);
  int (*task_getpgid)(struct task_struct *p);
  int (*task_getsid)(struct task_struct *p);
@@ -1876,6 +1887,7 @@ struct security_hook_heads {
  struct hlist_head kernel_post_read_file;
  struct hlist_head kernel_module_request;
  struct hlist_head task_fix_setuid;
+ struct hlist_head task_fix_setgid;
  struct hlist_head task_setpgid;
  struct hlist_head task_getpgid;
  struct hlist_head task_getsid;
diff --git a/include/linux/security.h b/include/linux/security.h
index 63030c85ee19..a82d97cf13ab 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -325,6 +325,8 @@ int security_kernel_post_read_file(struct file *file,
char *buf, loff_t size,
 enum kernel_read_file_id id);
 int security_task_fix_setuid(struct cred *new, const struct cred *old,
   int flags);
+int security_task_fix_setgid(struct cred *new, const struct cred *old,
+  int flags);
 int security_task_setpgid(struct task_struct *p, pid_t pgid);
 int security_task_getpgid(struct task_struct *p);
 int security_task_getsid(struct task_struct *p);
@@ -929,6 +931,13 @@ static inline int security_task_fix_setuid(struct cred
*new,
  return cap_task_fix_setuid(new, old, flags);
 }

+static inline int security_task_fix_setgid(struct cred *new,
+const struct cred *old,
+int flags)
+{
+ return 0;
+}
+
 static inline int security_task_setpgid(struct task_struct *p, pid_t pgid)
 {
  return 0;
diff --git a/kernel/sys.c b/kernel/sys.c
index 38509dc1f77b..f6ef922c6815 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -392,6 +392,10 @@ long __sys_setregid(gid_t rgid, gid_t egid)
  new->sgid = new->egid;
  new->fsgid = new->egid;

+ retval = security_task_fix_setgid(new, old, LSM_SETID_RE);
+ if (retval < 0)
+ goto error;
+
  return commit_creds(new);

 error:
@@ -434,6 +438,10 @@ long __sys_setgid(gid_t gid)
  else
  goto error;

+ retval = security_task_fix_setgid(new, old, LSM_SETID_ID);
+ if (retval < 0)
+ goto error;
+
  return commit_creds(new);

 error:
@@ -755,6 +763,10 @@ long __sys_setresgid(gid_t rgid, gid_t egid, gid_t
sgid)
  new->sgid = ksgid;
  new->fsgid = new->egid;

+ retval = security_task_fix_setgid(new, old, LSM_SETID_RES);
+ if (retval < 0)
+ goto error;
+
  return commit_creds(new);

 error:
@@ -861,7 +873,8 @@ long __sys_setfsgid(gid_t gid)
  ns_capable(old->user_ns, CAP_SETGID)) {
  if (!gid_eq(kgid, old->fsgid)) {
  new->fsgid = kgid;
- goto change_okay;
+ if (security_task_fix_setgid(new, old, LSM_SETID_FS) == 0)
+ goto change_okay;
  }
  }

diff --git a/security/security.c b/security/security.c
index 68f46d849abe..587786fc0aaa 100644
--- a/security/security.c
+++ b/secu