2023-02-28 16:53:17

by Paul Moore

[permalink] [raw]
Subject: Re: [PATCH testsuite 1/3] policy: make sure test_ibpkey_access_t can lock enough memory

On Tue, Feb 28, 2023 at 9:13 AM Ondrej Mosnacek <[email protected]> wrote:
>
> The ibv_create_cq() operation requires the caller to be able to lock
> enough memory (RLIMIT_MEMLOCK). In some environments (such as RHEL-8)
> the default resource limits may not be enough, requiring CAP_IPC_LOCK to
> go above the limit. To make sure the test works also under stricter
> resource limits, grant CAP_IPC_LOCK to test_ibpkey_access_t.
>
> Reported-by: Mimi Zohar <[email protected]>
> Signed-off-by: Ondrej Mosnacek <[email protected]>
> ---
> policy/test_ibpkey.te | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/policy/test_ibpkey.te b/policy/test_ibpkey.te
> index 863ff16..97f0c3c 100644
> --- a/policy/test_ibpkey.te
> +++ b/policy/test_ibpkey.te
> @@ -10,6 +10,8 @@ type test_ibpkey_access_t;
> testsuite_domain_type(test_ibpkey_access_t)
> typeattribute test_ibpkey_access_t ibpkeydomain;
>
> +allow test_ibpkey_access_t self:capability ipc_lock;

FWIW, I brought this up back in 2019 and have been carrying a local
selinux-testsuite patch for this ever since (it's the only way to get
a clean run of the IB tests). While it can be fixed in the
selinux-testsuite policy, I believe this is a more general problem and
should probably be fixed in refpol.

https://lore.kernel.org/selinux/CAHC9VhTuYi+W0RukEV4WNrP5X_AFeouaWMsdbgxSL1v04mouWw@mail.gmail.com/

> dev_rw_infiniband_dev(test_ibpkey_access_t)
> dev_rw_sysfs(test_ibpkey_access_t)
>
> --
> 2.39.2

--
paul-moore.com


2023-02-28 20:13:07

by Mimi Zohar

[permalink] [raw]
Subject: Re: [PATCH testsuite 1/3] policy: make sure test_ibpkey_access_t can lock enough memory

On Tue, 2023-02-28 at 11:51 -0500, Paul Moore wrote:
> On Tue, Feb 28, 2023 at 9:13 AM Ondrej Mosnacek <[email protected]> wrote:
> >
> > The ibv_create_cq() operation requires the caller to be able to lock
> > enough memory (RLIMIT_MEMLOCK). In some environments (such as RHEL-8)
> > the default resource limits may not be enough, requiring CAP_IPC_LOCK to
> > go above the limit. To make sure the test works also under stricter
> > resource limits, grant CAP_IPC_LOCK to test_ibpkey_access_t.
> >
> > Reported-by: Mimi Zohar <[email protected]>
> > Signed-off-by: Ondrej Mosnacek <[email protected]>
> > ---
> > policy/test_ibpkey.te | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/policy/test_ibpkey.te b/policy/test_ibpkey.te
> > index 863ff16..97f0c3c 100644
> > --- a/policy/test_ibpkey.te
> > +++ b/policy/test_ibpkey.te
> > @@ -10,6 +10,8 @@ type test_ibpkey_access_t;
> > testsuite_domain_type(test_ibpkey_access_t)
> > typeattribute test_ibpkey_access_t ibpkeydomain;
> >
> > +allow test_ibpkey_access_t self:capability ipc_lock;
>
> FWIW, I brought this up back in 2019 and have been carrying a local
> selinux-testsuite patch for this ever since (it's the only way to get
> a clean run of the IB tests).

Confirmed, with this change the SELinux infiniband tests are now
working on stable linux-4.19.y.

> While it can be fixed in the
> selinux-testsuite policy, I believe this is a more general problem and
> should probably be fixed in refpol.
>
> https://lore.kernel.org/selinux/CAHC9VhTuYi+W0RukEV4WNrP5X_AFeouaWMsdbgxSL1v04mouWw@mail.gmail.com/
>
> > dev_rw_infiniband_dev(test_ibpkey_access_t)
> > dev_rw_sysfs(test_ibpkey_access_t)

--
thanks,

Mimi



2023-03-01 15:22:01

by Ondrej Mosnacek

[permalink] [raw]
Subject: Re: [PATCH testsuite 1/3] policy: make sure test_ibpkey_access_t can lock enough memory

On Tue, Feb 28, 2023 at 5:51 PM Paul Moore <[email protected]> wrote:
> On Tue, Feb 28, 2023 at 9:13 AM Ondrej Mosnacek <[email protected]> wrote:
> >
> > The ibv_create_cq() operation requires the caller to be able to lock
> > enough memory (RLIMIT_MEMLOCK). In some environments (such as RHEL-8)
> > the default resource limits may not be enough, requiring CAP_IPC_LOCK to
> > go above the limit. To make sure the test works also under stricter
> > resource limits, grant CAP_IPC_LOCK to test_ibpkey_access_t.
> >
> > Reported-by: Mimi Zohar <[email protected]>
> > Signed-off-by: Ondrej Mosnacek <[email protected]>
> > ---
> > policy/test_ibpkey.te | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/policy/test_ibpkey.te b/policy/test_ibpkey.te
> > index 863ff16..97f0c3c 100644
> > --- a/policy/test_ibpkey.te
> > +++ b/policy/test_ibpkey.te
> > @@ -10,6 +10,8 @@ type test_ibpkey_access_t;
> > testsuite_domain_type(test_ibpkey_access_t)
> > typeattribute test_ibpkey_access_t ibpkeydomain;
> >
> > +allow test_ibpkey_access_t self:capability ipc_lock;
>
> FWIW, I brought this up back in 2019 and have been carrying a local
> selinux-testsuite patch for this ever since (it's the only way to get
> a clean run of the IB tests). While it can be fixed in the
> selinux-testsuite policy, I believe this is a more general problem and
> should probably be fixed in refpol.
>
> https://lore.kernel.org/selinux/CAHC9VhTuYi+W0RukEV4WNrP5X_AFeouaWMsdbgxSL1v04mouWw@mail.gmail.com/

I don't understand how you'd like this to be fixed in the system
policy... I don't think there is any policy interface that would
semantically match "any users of the SELinux IB hooks" or "callers of
ibv_create_cq()" that we could stick the capability rule into. At
least the testsuite policy doesn't use any such interface. Closest to
it would be dev_rw_infiniband_dev(), but that doesn't seem like the
right place.

Not to mention that the fact whether the capability is required or not
depends on the resource limits imposed on the process. If its
RLIMIT_MEMLOCK limit is sufficient, a process is perfectly able to
create the cq without CAP_IPC_LOCK. Automatically granting it to all
domains that use InfiniBand in some way "just in case" would
potentially grant it also to domains that don't actually need it,
violating the principle of least privilege.

--
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.


2023-03-01 18:49:17

by Paul Moore

[permalink] [raw]
Subject: Re: [PATCH testsuite 1/3] policy: make sure test_ibpkey_access_t can lock enough memory

On Wed, Mar 1, 2023 at 10:21 AM Ondrej Mosnacek <[email protected]> wrote:
> On Tue, Feb 28, 2023 at 5:51 PM Paul Moore <[email protected]> wrote:
> > On Tue, Feb 28, 2023 at 9:13 AM Ondrej Mosnacek <[email protected]> wrote:
> > >
> > > The ibv_create_cq() operation requires the caller to be able to lock
> > > enough memory (RLIMIT_MEMLOCK). In some environments (such as RHEL-8)
> > > the default resource limits may not be enough, requiring CAP_IPC_LOCK to
> > > go above the limit. To make sure the test works also under stricter
> > > resource limits, grant CAP_IPC_LOCK to test_ibpkey_access_t.
> > >
> > > Reported-by: Mimi Zohar <[email protected]>
> > > Signed-off-by: Ondrej Mosnacek <[email protected]>
> > > ---
> > > policy/test_ibpkey.te | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/policy/test_ibpkey.te b/policy/test_ibpkey.te
> > > index 863ff16..97f0c3c 100644
> > > --- a/policy/test_ibpkey.te
> > > +++ b/policy/test_ibpkey.te
> > > @@ -10,6 +10,8 @@ type test_ibpkey_access_t;
> > > testsuite_domain_type(test_ibpkey_access_t)
> > > typeattribute test_ibpkey_access_t ibpkeydomain;
> > >
> > > +allow test_ibpkey_access_t self:capability ipc_lock;
> >
> > FWIW, I brought this up back in 2019 and have been carrying a local
> > selinux-testsuite patch for this ever since (it's the only way to get
> > a clean run of the IB tests). While it can be fixed in the
> > selinux-testsuite policy, I believe this is a more general problem and
> > should probably be fixed in refpol.
> >
> > https://lore.kernel.org/selinux/CAHC9VhTuYi+W0RukEV4WNrP5X_AFeouaWMsdbgxSL1v04mouWw@mail.gmail.com/
>
> I don't understand how you'd like this to be fixed in the system
> policy... I don't think there is any policy interface that would
> semantically match "any users of the SELinux IB hooks" or "callers of
> ibv_create_cq()" that we could stick the capability rule into. At
> least the testsuite policy doesn't use any such interface. Closest to
> it would be dev_rw_infiniband_dev(), but that doesn't seem like the
> right place.

Look at it this way, the selinux-testsuite is not doing anything
particularly unusual with respect to talking over IB; if the tests
need that permission it seems reasonable that normal IB users would
also need these permissions.

> Not to mention that the fact whether the capability is required or not
> depends on the resource limits imposed on the process. If its
> RLIMIT_MEMLOCK limit is sufficient, a process is perfectly able to
> create the cq without CAP_IPC_LOCK. Automatically granting it to all
> domains that use InfiniBand in some way "just in case" would
> potentially grant it also to domains that don't actually need it,
> violating the principle of least privilege.

Once again, the selinux-testsuite is not doing anything particularly
unusual so if we are hitting this it seems reasonable that other users
are hitting this as well. If you're concerned about granting
CAP_IPC_LOCK you could always put it in a dedicated IB/RDMA refpol
interface as I believe this is just an issue with the IB/RDMA verb
interface involving CQs/QPs and not the underlying IB protocol layer.
Say something like "dev_rw_infiniband_rdma()"* which would call
"dev_rw_infiniband()"* and add the CAP_IPC_LOCK permission.

It would be good to hear Chris' take on this.

* Upstream refpol appears to have shortened the interface to
"dev_rw_infiniband()".

--
paul-moore.com