* [PATCH 5.10 130/154] block: Check ADMIN before NICE for IOPRIO_CLASS_RT
[not found] <20211124115702.361983534@linuxfoundation.org>
@ 2021-11-24 11:58 ` Greg Kroah-Hartman
2021-11-24 14:22 ` Jari Ruusu
0 siblings, 1 reply; 8+ messages in thread
From: Greg Kroah-Hartman @ 2021-11-24 11:58 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kroah-Hartman, stable, Alistair Delva, Khazhismel Kumykov,
Bart Van Assche, Serge Hallyn, Jens Axboe, Paul Moore, selinux,
linux-security-module, kernel-team
From: Alistair Delva <adelva@google.com>
commit 94c4b4fd25e6c3763941bdec3ad54f2204afa992 upstream.
Booting to Android userspace on 5.14 or newer triggers the following
SELinux denial:
avc: denied { sys_nice } for comm="init" capability=23
scontext=u:r:init:s0 tcontext=u:r:init:s0 tclass=capability
permissive=0
Init is PID 0 running as root, so it already has CAP_SYS_ADMIN. For
better compatibility with older SEPolicy, check ADMIN before NICE.
Fixes: 9d3a39a5f1e4 ("block: grant IOPRIO_CLASS_RT to CAP_SYS_NICE")
Signed-off-by: Alistair Delva <adelva@google.com>
Cc: Khazhismel Kumykov <khazhy@google.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Paul Moore <paul@paul-moore.com>
Cc: selinux@vger.kernel.org
Cc: linux-security-module@vger.kernel.org
Cc: kernel-team@android.com
Cc: stable@vger.kernel.org # v5.14+
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Acked-by: Serge Hallyn <serge@hallyn.com>
Link: https://lore.kernel.org/r/20211115181655.3608659-1-adelva@google.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
block/ioprio.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
--- a/block/ioprio.c
+++ b/block/ioprio.c
@@ -69,7 +69,14 @@ int ioprio_check_cap(int ioprio)
switch (class) {
case IOPRIO_CLASS_RT:
- if (!capable(CAP_SYS_NICE) && !capable(CAP_SYS_ADMIN))
+ /*
+ * Originally this only checked for CAP_SYS_ADMIN,
+ * which was implicitly allowed for pid 0 by security
+ * modules such as SELinux. Make sure we check
+ * CAP_SYS_ADMIN first to avoid a denial/avc for
+ * possibly missing CAP_SYS_NICE permission.
+ */
+ if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_NICE))
return -EPERM;
fallthrough;
/* rt has prio field too */
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 5.10 130/154] block: Check ADMIN before NICE for IOPRIO_CLASS_RT
2021-11-24 11:58 ` [PATCH 5.10 130/154] block: Check ADMIN before NICE for IOPRIO_CLASS_RT Greg Kroah-Hartman
@ 2021-11-24 14:22 ` Jari Ruusu
2021-11-24 15:31 ` Greg Kroah-Hartman
0 siblings, 1 reply; 8+ messages in thread
From: Jari Ruusu @ 2021-11-24 14:22 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, stable, Alistair Delva, Khazhismel Kumykov,
Bart Van Assche, Serge Hallyn, Jens Axboe, Paul Moore, selinux,
linux-security-module, kernel-team
Greg Kroah-Hartman wrote:
> From: Alistair Delva <adelva@google.com>
>
> commit 94c4b4fd25e6c3763941bdec3ad54f2204afa992 upstream.
[SNIP]
> --- a/block/ioprio.c
> +++ b/block/ioprio.c
> @@ -69,7 +69,14 @@ int ioprio_check_cap(int ioprio)
>
> switch (class) {
> case IOPRIO_CLASS_RT:
> - if (!capable(CAP_SYS_NICE) && !capable(CAP_SYS_ADMIN))
> + /*
> + * Originally this only checked for CAP_SYS_ADMIN,
> + * which was implicitly allowed for pid 0 by security
> + * modules such as SELinux. Make sure we check
> + * CAP_SYS_ADMIN first to avoid a denial/avc for
> + * possibly missing CAP_SYS_NICE permission.
> + */
> + if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_NICE))
> return -EPERM;
> fallthrough;
> /* rt has prio field too */
What exactly is above patch trying to fix?
It does not change control flow at all, and added comment is misleading.
--
Jari Ruusu 4096R/8132F189 12D6 4C3A DCDA 0AA4 27BD ACDF F073 3C80 8132 F189
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 5.10 130/154] block: Check ADMIN before NICE for IOPRIO_CLASS_RT
2021-11-24 14:22 ` Jari Ruusu
@ 2021-11-24 15:31 ` Greg Kroah-Hartman
2021-11-24 17:33 ` Serge E. Hallyn
0 siblings, 1 reply; 8+ messages in thread
From: Greg Kroah-Hartman @ 2021-11-24 15:31 UTC (permalink / raw)
To: Jari Ruusu
Cc: linux-kernel, stable, Alistair Delva, Khazhismel Kumykov,
Bart Van Assche, Serge Hallyn, Jens Axboe, Paul Moore, selinux,
linux-security-module, kernel-team
On Wed, Nov 24, 2021 at 04:22:50PM +0200, Jari Ruusu wrote:
> Greg Kroah-Hartman wrote:
> > From: Alistair Delva <adelva@google.com>
> >
> > commit 94c4b4fd25e6c3763941bdec3ad54f2204afa992 upstream.
> [SNIP]
> > --- a/block/ioprio.c
> > +++ b/block/ioprio.c
> > @@ -69,7 +69,14 @@ int ioprio_check_cap(int ioprio)
> >
> > switch (class) {
> > case IOPRIO_CLASS_RT:
> > - if (!capable(CAP_SYS_NICE) && !capable(CAP_SYS_ADMIN))
> > + /*
> > + * Originally this only checked for CAP_SYS_ADMIN,
> > + * which was implicitly allowed for pid 0 by security
> > + * modules such as SELinux. Make sure we check
> > + * CAP_SYS_ADMIN first to avoid a denial/avc for
> > + * possibly missing CAP_SYS_NICE permission.
> > + */
> > + if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_NICE))
> > return -EPERM;
> > fallthrough;
> > /* rt has prio field too */
>
> What exactly is above patch trying to fix?
> It does not change control flow at all, and added comment is misleading.
See the thread on the mailing list for what it does and why it is
needed.
It does change the result when selinux is enabled.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 5.10 130/154] block: Check ADMIN before NICE for IOPRIO_CLASS_RT
2021-11-24 15:31 ` Greg Kroah-Hartman
@ 2021-11-24 17:33 ` Serge E. Hallyn
2021-11-24 18:15 ` Greg Kroah-Hartman
0 siblings, 1 reply; 8+ messages in thread
From: Serge E. Hallyn @ 2021-11-24 17:33 UTC (permalink / raw)
To: Greg Kroah-Hartman, serge
Cc: Jari Ruusu, linux-kernel, stable, Alistair Delva,
Khazhismel Kumykov, Bart Van Assche, Serge Hallyn, Jens Axboe,
Paul Moore, selinux, linux-security-module, kernel-team
On Wed, Nov 24, 2021 at 04:31:22PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Nov 24, 2021 at 04:22:50PM +0200, Jari Ruusu wrote:
> > Greg Kroah-Hartman wrote:
> > > From: Alistair Delva <adelva@google.com>
> > >
> > > commit 94c4b4fd25e6c3763941bdec3ad54f2204afa992 upstream.
> > [SNIP]
> > > --- a/block/ioprio.c
> > > +++ b/block/ioprio.c
> > > @@ -69,7 +69,14 @@ int ioprio_check_cap(int ioprio)
> > >
> > > switch (class) {
> > > case IOPRIO_CLASS_RT:
> > > - if (!capable(CAP_SYS_NICE) && !capable(CAP_SYS_ADMIN))
> > > + /*
> > > + * Originally this only checked for CAP_SYS_ADMIN,
> > > + * which was implicitly allowed for pid 0 by security
> > > + * modules such as SELinux. Make sure we check
> > > + * CAP_SYS_ADMIN first to avoid a denial/avc for
> > > + * possibly missing CAP_SYS_NICE permission.
> > > + */
> > > + if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_NICE))
> > > return -EPERM;
> > > fallthrough;
> > > /* rt has prio field too */
> >
> > What exactly is above patch trying to fix?
> > It does not change control flow at all, and added comment is misleading.
>
> See the thread on the mailing list for what it does and why it is
> needed.
>
> It does change the result when selinux is enabled.
>
> thanks,
>
> greg k-h
The case where we create a newer more fine grained capability which is a
sub-cap of a broader capability like CAP_SYS_ADMIN is analogous. See
check_syslog_permissions() for instance.
So I think a helper like
int capable_either_or(int cap1, int cap2) {
if (has_capability_noaudit(current, cap1))
return 0;
return capable(cap2);
}
might be worthwhile.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 5.10 130/154] block: Check ADMIN before NICE for IOPRIO_CLASS_RT
2021-11-24 17:33 ` Serge E. Hallyn
@ 2021-11-24 18:15 ` Greg Kroah-Hartman
2021-11-24 18:34 ` Christian Göttsche
2021-11-24 23:29 ` Serge E. Hallyn
0 siblings, 2 replies; 8+ messages in thread
From: Greg Kroah-Hartman @ 2021-11-24 18:15 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Jari Ruusu, linux-kernel, stable, Alistair Delva,
Khazhismel Kumykov, Bart Van Assche, Jens Axboe, Paul Moore,
selinux, linux-security-module, kernel-team
On Wed, Nov 24, 2021 at 11:33:11AM -0600, Serge E. Hallyn wrote:
> On Wed, Nov 24, 2021 at 04:31:22PM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Nov 24, 2021 at 04:22:50PM +0200, Jari Ruusu wrote:
> > > Greg Kroah-Hartman wrote:
> > > > From: Alistair Delva <adelva@google.com>
> > > >
> > > > commit 94c4b4fd25e6c3763941bdec3ad54f2204afa992 upstream.
> > > [SNIP]
> > > > --- a/block/ioprio.c
> > > > +++ b/block/ioprio.c
> > > > @@ -69,7 +69,14 @@ int ioprio_check_cap(int ioprio)
> > > >
> > > > switch (class) {
> > > > case IOPRIO_CLASS_RT:
> > > > - if (!capable(CAP_SYS_NICE) && !capable(CAP_SYS_ADMIN))
> > > > + /*
> > > > + * Originally this only checked for CAP_SYS_ADMIN,
> > > > + * which was implicitly allowed for pid 0 by security
> > > > + * modules such as SELinux. Make sure we check
> > > > + * CAP_SYS_ADMIN first to avoid a denial/avc for
> > > > + * possibly missing CAP_SYS_NICE permission.
> > > > + */
> > > > + if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_NICE))
> > > > return -EPERM;
> > > > fallthrough;
> > > > /* rt has prio field too */
> > >
> > > What exactly is above patch trying to fix?
> > > It does not change control flow at all, and added comment is misleading.
> >
> > See the thread on the mailing list for what it does and why it is
> > needed.
> >
> > It does change the result when selinux is enabled.
> >
> > thanks,
> >
> > greg k-h
>
> The case where we create a newer more fine grained capability which is a
> sub-cap of a broader capability like CAP_SYS_ADMIN is analogous. See
> check_syslog_permissions() for instance.
>
> So I think a helper like
>
> int capable_either_or(int cap1, int cap2) {
> if (has_capability_noaudit(current, cap1))
> return 0;
> return capable(cap2);
> }
>
> might be worthwhile.
Sure, feel free to work on that and submit it, but for now, this change
is needed.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 5.10 130/154] block: Check ADMIN before NICE for IOPRIO_CLASS_RT
2021-11-24 18:15 ` Greg Kroah-Hartman
@ 2021-11-24 18:34 ` Christian Göttsche
2021-11-24 23:30 ` Serge E. Hallyn
2021-11-24 23:29 ` Serge E. Hallyn
1 sibling, 1 reply; 8+ messages in thread
From: Christian Göttsche @ 2021-11-24 18:34 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Serge E. Hallyn, Jari Ruusu, linux-kernel, stable, Alistair Delva,
Khazhismel Kumykov, Bart Van Assche, Jens Axboe, Paul Moore,
SElinux list, linux-security-module, kernel-team
On Wed, 24 Nov 2021 at 19:16, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Nov 24, 2021 at 11:33:11AM -0600, Serge E. Hallyn wrote:
> > On Wed, Nov 24, 2021 at 04:31:22PM +0100, Greg Kroah-Hartman wrote:
> > > On Wed, Nov 24, 2021 at 04:22:50PM +0200, Jari Ruusu wrote:
> > > > Greg Kroah-Hartman wrote:
> > > > > From: Alistair Delva <adelva@google.com>
> > > > >
> > > > > commit 94c4b4fd25e6c3763941bdec3ad54f2204afa992 upstream.
> > > > [SNIP]
> > > > > --- a/block/ioprio.c
> > > > > +++ b/block/ioprio.c
> > > > > @@ -69,7 +69,14 @@ int ioprio_check_cap(int ioprio)
> > > > >
> > > > > switch (class) {
> > > > > case IOPRIO_CLASS_RT:
> > > > > - if (!capable(CAP_SYS_NICE) && !capable(CAP_SYS_ADMIN))
> > > > > + /*
> > > > > + * Originally this only checked for CAP_SYS_ADMIN,
> > > > > + * which was implicitly allowed for pid 0 by security
> > > > > + * modules such as SELinux. Make sure we check
> > > > > + * CAP_SYS_ADMIN first to avoid a denial/avc for
> > > > > + * possibly missing CAP_SYS_NICE permission.
> > > > > + */
> > > > > + if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_NICE))
> > > > > return -EPERM;
> > > > > fallthrough;
> > > > > /* rt has prio field too */
> > > >
> > > > What exactly is above patch trying to fix?
> > > > It does not change control flow at all, and added comment is misleading.
> > >
> > > See the thread on the mailing list for what it does and why it is
> > > needed.
> > >
> > > It does change the result when selinux is enabled.
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > The case where we create a newer more fine grained capability which is a
> > sub-cap of a broader capability like CAP_SYS_ADMIN is analogous. See
> > check_syslog_permissions() for instance.
> >
> > So I think a helper like
> >
> > int capable_either_or(int cap1, int cap2) {
> > if (has_capability_noaudit(current, cap1))
> > return 0;
> > return capable(cap2);
> > }
> >
> > might be worthwhile.
>
I proposed an early prototype at
https://patchwork.kernel.org/project/selinux/patch/20211116112437.43412-1-cgzones@googlemail.com/
> Sure, feel free to work on that and submit it, but for now, this change
> is needed.
>
I would argue this change is not necessary since the actual syscall
still succeeds as this is only an informative avc denial message about
a failed capability check. But this ship has sailed...
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 5.10 130/154] block: Check ADMIN before NICE for IOPRIO_CLASS_RT
2021-11-24 18:15 ` Greg Kroah-Hartman
2021-11-24 18:34 ` Christian Göttsche
@ 2021-11-24 23:29 ` Serge E. Hallyn
1 sibling, 0 replies; 8+ messages in thread
From: Serge E. Hallyn @ 2021-11-24 23:29 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Serge E. Hallyn, Jari Ruusu, linux-kernel, stable, Alistair Delva,
Khazhismel Kumykov, Bart Van Assche, Jens Axboe, Paul Moore,
selinux, linux-security-module, kernel-team
On Wed, Nov 24, 2021 at 07:15:35PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Nov 24, 2021 at 11:33:11AM -0600, Serge E. Hallyn wrote:
> > On Wed, Nov 24, 2021 at 04:31:22PM +0100, Greg Kroah-Hartman wrote:
> > > On Wed, Nov 24, 2021 at 04:22:50PM +0200, Jari Ruusu wrote:
> > > > Greg Kroah-Hartman wrote:
> > > > > From: Alistair Delva <adelva@google.com>
> > > > >
> > > > > commit 94c4b4fd25e6c3763941bdec3ad54f2204afa992 upstream.
> > > > [SNIP]
> > > > > --- a/block/ioprio.c
> > > > > +++ b/block/ioprio.c
> > > > > @@ -69,7 +69,14 @@ int ioprio_check_cap(int ioprio)
> > > > >
> > > > > switch (class) {
> > > > > case IOPRIO_CLASS_RT:
> > > > > - if (!capable(CAP_SYS_NICE) && !capable(CAP_SYS_ADMIN))
> > > > > + /*
> > > > > + * Originally this only checked for CAP_SYS_ADMIN,
> > > > > + * which was implicitly allowed for pid 0 by security
> > > > > + * modules such as SELinux. Make sure we check
> > > > > + * CAP_SYS_ADMIN first to avoid a denial/avc for
> > > > > + * possibly missing CAP_SYS_NICE permission.
> > > > > + */
> > > > > + if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_NICE))
> > > > > return -EPERM;
> > > > > fallthrough;
> > > > > /* rt has prio field too */
> > > >
> > > > What exactly is above patch trying to fix?
> > > > It does not change control flow at all, and added comment is misleading.
> > >
> > > See the thread on the mailing list for what it does and why it is
> > > needed.
> > >
> > > It does change the result when selinux is enabled.
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > The case where we create a newer more fine grained capability which is a
> > sub-cap of a broader capability like CAP_SYS_ADMIN is analogous. See
> > check_syslog_permissions() for instance.
> >
> > So I think a helper like
> >
> > int capable_either_or(int cap1, int cap2) {
> > if (has_capability_noaudit(current, cap1))
> > return 0;
> > return capable(cap2);
> > }
> >
> > might be worthwhile.
>
> Sure, feel free to work on that and submit it, but for now, this change
> is needed.
Sorry I misread the subject and thought this was just a resubmission.
FWIW I had acked an earlier version of this.
-serge
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 5.10 130/154] block: Check ADMIN before NICE for IOPRIO_CLASS_RT
2021-11-24 18:34 ` Christian Göttsche
@ 2021-11-24 23:30 ` Serge E. Hallyn
0 siblings, 0 replies; 8+ messages in thread
From: Serge E. Hallyn @ 2021-11-24 23:30 UTC (permalink / raw)
To: Christian Göttsche
Cc: Greg Kroah-Hartman, Serge E. Hallyn, Jari Ruusu, linux-kernel,
stable, Alistair Delva, Khazhismel Kumykov, Bart Van Assche,
Jens Axboe, Paul Moore, SElinux list, linux-security-module,
kernel-team
On Wed, Nov 24, 2021 at 07:34:50PM +0100, Christian Göttsche wrote:
> On Wed, 24 Nov 2021 at 19:16, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Nov 24, 2021 at 11:33:11AM -0600, Serge E. Hallyn wrote:
> > > On Wed, Nov 24, 2021 at 04:31:22PM +0100, Greg Kroah-Hartman wrote:
> > > > On Wed, Nov 24, 2021 at 04:22:50PM +0200, Jari Ruusu wrote:
> > > > > Greg Kroah-Hartman wrote:
> > > > > > From: Alistair Delva <adelva@google.com>
> > > > > >
> > > > > > commit 94c4b4fd25e6c3763941bdec3ad54f2204afa992 upstream.
> > > > > [SNIP]
> > > > > > --- a/block/ioprio.c
> > > > > > +++ b/block/ioprio.c
> > > > > > @@ -69,7 +69,14 @@ int ioprio_check_cap(int ioprio)
> > > > > >
> > > > > > switch (class) {
> > > > > > case IOPRIO_CLASS_RT:
> > > > > > - if (!capable(CAP_SYS_NICE) && !capable(CAP_SYS_ADMIN))
> > > > > > + /*
> > > > > > + * Originally this only checked for CAP_SYS_ADMIN,
> > > > > > + * which was implicitly allowed for pid 0 by security
> > > > > > + * modules such as SELinux. Make sure we check
> > > > > > + * CAP_SYS_ADMIN first to avoid a denial/avc for
> > > > > > + * possibly missing CAP_SYS_NICE permission.
> > > > > > + */
> > > > > > + if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_NICE))
> > > > > > return -EPERM;
> > > > > > fallthrough;
> > > > > > /* rt has prio field too */
> > > > >
> > > > > What exactly is above patch trying to fix?
> > > > > It does not change control flow at all, and added comment is misleading.
> > > >
> > > > See the thread on the mailing list for what it does and why it is
> > > > needed.
> > > >
> > > > It does change the result when selinux is enabled.
> > > >
> > > > thanks,
> > > >
> > > > greg k-h
> > >
> > > The case where we create a newer more fine grained capability which is a
> > > sub-cap of a broader capability like CAP_SYS_ADMIN is analogous. See
> > > check_syslog_permissions() for instance.
> > >
> > > So I think a helper like
> > >
> > > int capable_either_or(int cap1, int cap2) {
> > > if (has_capability_noaudit(current, cap1))
> > > return 0;
> > > return capable(cap2);
> > > }
> > >
> > > might be worthwhile.
> >
>
> I proposed an early prototype at
> https://patchwork.kernel.org/project/selinux/patch/20211116112437.43412-1-cgzones@googlemail.com/
I never saw this. Would you mind resending as a standalone patch?
(I do have comments, but this thread seems the wrong place)
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-11-24 23:30 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20211124115702.361983534@linuxfoundation.org>
2021-11-24 11:58 ` [PATCH 5.10 130/154] block: Check ADMIN before NICE for IOPRIO_CLASS_RT Greg Kroah-Hartman
2021-11-24 14:22 ` Jari Ruusu
2021-11-24 15:31 ` Greg Kroah-Hartman
2021-11-24 17:33 ` Serge E. Hallyn
2021-11-24 18:15 ` Greg Kroah-Hartman
2021-11-24 18:34 ` Christian Göttsche
2021-11-24 23:30 ` Serge E. Hallyn
2021-11-24 23:29 ` Serge E. Hallyn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).