mbox series

[GIT,PULL] SELinux fixes for v5.7 (#2)

Message ID CAHC9VhTu3YWPmwtA7RERHDRhQt0wAkmN4GJCmaRY7KSFRwtACQ@mail.gmail.com (mailing list archive)
State Accepted
Headers show
Series [GIT,PULL] SELinux fixes for v5.7 (#2) | expand

Pull-request

git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20200430

Message

Paul Moore April 30, 2020, 9:24 p.m. UTC
Hi Linus,

Two more SELinux patches to fix problems in the v5.7-rcX releases.
Wei Yongjun's patch fixes a return code in an error path, and my patch
fixes a problem where we were not correctly applying access controls
to all of the netlink messages in the netlink_send LSM hook.  Both
patches pass our tests without problem and currently apply cleanly on
top of your master branch.  Please merge for the next -rcX release.

Thanks,
-Paul

--
The following changes since commit af15f14c8cfcee515f4e9078889045ad63efefe3:

 selinux: free str on error in str_read() (2020-04-15 17:23:16 -0400)

are available in the Git repository at:

 git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
   tags/selinux-pr-20200430

for you to fetch changes up to fb73974172ffaaf57a7c42f35424d9aece1a5af6:

 selinux: properly handle multiple messages in selinux_netlink_send()
   (2020-04-30 16:18:37 -0400)

----------------------------------------------------------------
selinux/stable-5.7 PR 20200430

----------------------------------------------------------------
Paul Moore (1):
     selinux: properly handle multiple messages in selinux_netlink_send()

Wei Yongjun (1):
     selinux: fix error return code in cond_read_list()

security/selinux/hooks.c          | 70 +++++++++++++++++++++----------
security/selinux/ss/conditional.c |  2 +-
2 files changed, 46 insertions(+), 26 deletions(-)

Comments

Linus Torvalds April 30, 2020, 11:43 p.m. UTC | #1
On Thu, Apr 30, 2020 at 2:24 PM Paul Moore <paul@paul-moore.com> wrote:
>
> Two more SELinux patches to fix problems in the v5.7-rcX releases.
> Wei Yongjun's patch fixes a return code in an error path, and my patch
> fixes a problem where we were not correctly applying access controls
> to all of the netlink messages in the netlink_send LSM hook.

Side note: could we plan on (not for 5.7, but future) moving the "for
each message" part of that patch into the generic security layer (ie
security_netlink_send()), so that if/when other security subsystems
start doing that netlink thing, they won't have to duplicate that
code?

Obviously the "for each message" thing should only be done if there is
any security  hook at all..

Right now selinux is the only one that does this, so there's no
duplication of effort, but it seems a mistake to do this at the
low-level security level.

Or is there some fundamental reason why a security hook would want to
look at a single skb rather than the individual messages?

                Linus
pr-tracker-bot@kernel.org April 30, 2020, 11:50 p.m. UTC | #2
The pull request you sent on Thu, 30 Apr 2020 17:24:20 -0400:

> git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20200430

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/39e16d93424b61e0b5bd182e308a56d5f0e489d6

Thank you!
Paul Moore May 1, 2020, 4:17 p.m. UTC | #3
On Thu, Apr 30, 2020 at 7:43 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Apr 30, 2020 at 2:24 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > Two more SELinux patches to fix problems in the v5.7-rcX releases.
> > Wei Yongjun's patch fixes a return code in an error path, and my patch
> > fixes a problem where we were not correctly applying access controls
> > to all of the netlink messages in the netlink_send LSM hook.
>
> Side note: could we plan on (not for 5.7, but future) moving the "for
> each message" part of that patch into the generic security layer (ie
> security_netlink_send()), so that if/when other security subsystems
> start doing that netlink thing, they won't have to duplicate that
> code?
>
> Obviously the "for each message" thing should only be done if there is
> any security  hook at all..
>
> Right now selinux is the only one that does this, so there's no
> duplication of effort, but it seems a mistake to do this at the
> low-level security level.
>
> Or is there some fundamental reason why a security hook would want to
> look at a single skb rather than the individual messages?

Off the top of my head I can't think of why a LSM would want to look
only at the skb instead of the individual netlink messages.  I suppose
if that ever becomes an issue we could always pass the skb to the hook
along with the nlmsghdr, and the LSM would just need to deal with
being called multiple times for the same skb.  Another option might be
to give the LSM the option of registering one of two hooks for the
netlink_send hook; one type of hook would behave the same as the hook
does now, the other type would be called once for each message in the
skb.  Although this second option seems like a lot of extra complexity
for a questionable advantage, especially since only SELinux is using
the hook at the moment and we can easily change the hook without
breaking things.

It's also worth mentioning that we've always tried to keep the hook
layer (the stuff in security/security.c) relatively thin, but that's a
battle we've been slowly losing over time.  Moving the skb/nlmsghdr
processing into security_netlink_send() seems reasonable given some of
the other hooks.

Regardless, I'll work on something for an upcoming merge window, stay tuned.