Re: [PATCH rdma-next] IB/core: Don't register MAD agents for LSM notifications

[Paul Moore <paul@paul-moore.com>]

On Mon, Jan 28, 2019 at 9:57 PM Don Dutile <ddutile@redhat.com> wrote:
> On 01/28/2019 06:03 PM, Paul Moore wrote:
> > On Mon, Jan 28, 2019 at 11:57 AM Daniel Jurgens <danielj@mellanox.com> wrote:
> >> On 1/28/2019 10:37 AM, Paul Moore wrote:
> >>> On Sun, Jan 27, 2019 at 3:10 AM Leon Romanovsky <leon@kernel.org> wrote:
> >>>> From: Daniel Jurgens <danielj@mellanox.com>
> >>>>
> >>>> ---
> >>>>   drivers/infiniband/core/security.c | 34 ++++--------------------------
> >>>>   include/rdma/ib_mad.h              |  3 ---
> >>>>   2 files changed, 4 insertions(+), 33 deletions(-)
> >>> Perhaps predictably, I'm not very excited about this change.  Have you
> >>> looked closer into the slowdown to see where the cycles are being
> >>> spent?  I'm wondering if the issue is that a large number of notifiers
> >>> are being registered with the same priority causing the while loop in
> >>> notifier_chain_register() to take a significant amount of time.
> >>
> >> That's what's happening, each MAD agent is registering it's own notifier. The bug reporter was creating hundreds or thousands of  short lived MAD agents. With IRQs disabled too long it resulted in timeouts.
> >>
> >> When I initially added the notifier mechanism I thought it was you that said it wasn't really needed, since access wasn't generally revoked in these types of scenarios. Given that I didn't think this would be especially controversial. It was nice to have, unfortunately it causes problems even for users that don't enable SELinux.
> >
> > Revoking permission is difficult, and in some cases likely impossible,
> > but that doesn't mean we shouldn't try to make it possible when we
> > can.  I'd like to see if we can sort this out before we give up and
> > rip it out.
>
> Agree that it'd be nice to see it work, but since few (no other?) subsys registered
> with selinux/security is doing this, can we 'release' the IB mad agents from this
> 'imprisonment' and figure out a general solution that resolves this issue, and
> then it can be implemented not only for ib mad packet access, but for other
> subsys using selinux/security, and wanting the same revocation support.

If you scroll up, ever so slightly to my last reply (it's just a few
lines, I'll wait), you'll see where I said "I'd like to see if we can
sort this out before we give up and rip it out."

I don't recall at the moment why the notifier approach was taken with
the IB hooks back when Daniel wrote the code, and I'm definitely not
sure what other subsystems you are using for your comparisons here,
but I can tell you that I am beyond sick and tired of hearing people
immediately jump to "rip out this security code" because of some
performance glitch.  I care about performance too, okay?  We've got a
bunch of clever people here, let's spend some time and see if we can
improve what we've got before we make some knee jerk reaction and just
rip out the access controls.

> > I'm trying to quickly understand the MAD agent lifecycle, and it looks
> > like you have your own register/unregister routines, with locking, so
> > is it reasonable to assume that it would be possible to iterate over
> > the MAD agents in the IB code?  I wonder if it would be possible to
> > group MAD agents (per-port grouping, does that make sense?) such that
> > several agents would share a single LSM notifier registration?
> >
> one can have numerous MAD agents.  it isn't just one.

Clearly.  Daniel already mentioned hundreds of thousands of MAD agents.

> Think: you can have any number of apps scanning the fabric for various stats
>         to draw node/graph pics, gather per-port packet counts, gather bw numbers,
>         etc.  For IB, scanning the fabric involves 'mad pkts'.
>         In addition to scanning, one could do 'less then nice things' to switches,
>         not just read stats but set parameters... like routing table entries.
>         and thus, the security level addition.
>        One is highly unlikely to grant access to an app to do mad pkt generation/use,
>        then change your mind (semi-?)randomly to enable/disable it.... generally always enabled,
>        or always disabled.
>
> Let's take this case as a catalyst to resolve a previously unknown perf issue,
> and not hold it up to a higher functional requirement then other apps that want to be secured.

See my comment above.  I don't even know what you are talking about
regarding "higher functional arguments", what other subsystems?
Seriously Don, language like this causes an extreme allergic reaction.

Anyway, let's move on to something a bit more constructive, shall we?

From what I've gathered from this thread the issue stems from the
ib_mad_agent_security_setup() where we register the
ib_mad_agent_security_change() callback with the LSM notification
mechanism.  Looking at the ib_mad_agent_security_change() function,
all it seems to do is perform a LSM permission check
(security_ib_endport_manage_subnet()) and store the result in
ib_mad_agent->smp_allowed; quickly grep'ing through the code, the only
place where I see that value used is in ib_mad_enforce_security().

Looking at ib_mad_enforce_security(), it seems to have access to the
necessary MAD agent information, via ib_mad_agent_private->agent, to
perform the LSM permission check directly in this function.  Could we
not get rid of the MAD agent LSM notification callback and simply do
the LSM access check directly in ib_mad_enforce_security()?  What am I
missing?

I do see that the SELinux implementation of the
security_ib_endport_manage_subnet() LSM hook doesn't seem to cache the
IB endport labels.  If that proves to be a performance issue we can
create a cache for that (like we do for the pkeys and a few other
objects).

I barely understand IB, so it is very possible there is a fundamental
flaw in the logic above, but let's try to work together and figure
something out.  The above may not be it, but I'm not ready to throw in
the towel.

-- 
paul moore
www.paul-moore.com