linux-xfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RESEND] xfs: don't generate selinux audit messages for capability testing
@ 2022-03-01  2:50 Darrick J. Wong
  2022-03-01 15:10 ` Serge E. Hallyn
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Darrick J. Wong @ 2022-03-01  2:50 UTC (permalink / raw)
  To: xfs
  Cc: Ondrej Mosnacek, Dave Chinner, linux-security-module, selinux,
	john.haxby

From: Darrick J. Wong <djwong@kernel.org>

There are a few places where we test the current process' capability set
to decide if we're going to be more or less generous with resource
acquisition for a system call.  If the process doesn't have the
capability, we can continue the call, albeit in a degraded mode.

These are /not/ the actual security decisions, so it's not proper to use
capable(), which (in certain selinux setups) causes audit messages to
get logged.  Switch them to has_capability_noaudit.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Cc: Ondrej Mosnacek <omosnace@redhat.com>
Cc: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/xfs_fsmap.c  |    4 ++--
 fs/xfs/xfs_ioctl.c  |    2 +-
 fs/xfs/xfs_iops.c   |    2 +-
 kernel/capability.c |    1 +
 4 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
index 48287caad28b..10e1cb71439e 100644
--- a/fs/xfs/xfs_fsmap.c
+++ b/fs/xfs/xfs_fsmap.c
@@ -864,8 +864,8 @@ xfs_getfsmap(
 	    !xfs_getfsmap_is_valid_device(mp, &head->fmh_keys[1]))
 		return -EINVAL;
 
-	use_rmap = capable(CAP_SYS_ADMIN) &&
-		   xfs_has_rmapbt(mp);
+	use_rmap = xfs_has_rmapbt(mp) &&
+		   has_capability_noaudit(current, CAP_SYS_ADMIN);
 	head->fmh_entries = 0;
 
 	/* Set up our device handlers. */
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 2515fe8299e1..83481005317a 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1189,7 +1189,7 @@ xfs_ioctl_setattr_get_trans(
 		goto out_error;
 
 	error = xfs_trans_alloc_ichange(ip, NULL, NULL, pdqp,
-			capable(CAP_FOWNER), &tp);
+			has_capability_noaudit(current, CAP_FOWNER), &tp);
 	if (error)
 		goto out_error;
 
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index b79b3846e71b..a65217f787cf 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -723,7 +723,7 @@ xfs_setattr_nonsize(
 	}
 
 	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL,
-			capable(CAP_FOWNER), &tp);
+			has_capability_noaudit(current, CAP_FOWNER), &tp);
 	if (error)
 		goto out_dqrele;
 
diff --git a/kernel/capability.c b/kernel/capability.c
index 46a361dde042..765194f5d678 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -360,6 +360,7 @@ bool has_capability_noaudit(struct task_struct *t, int cap)
 {
 	return has_ns_capability_noaudit(t, &init_user_ns, cap);
 }
+EXPORT_SYMBOL(has_capability_noaudit);
 
 static bool ns_capable_common(struct user_namespace *ns,
 			      int cap,

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH RESEND] xfs: don't generate selinux audit messages for capability testing
  2022-03-01  2:50 [PATCH RESEND] xfs: don't generate selinux audit messages for capability testing Darrick J. Wong
@ 2022-03-01 15:10 ` Serge E. Hallyn
  2022-03-01 15:48   ` Darrick J. Wong
  2022-03-02 11:01 ` Ondrej Mosnacek
  2022-03-03 17:21 ` Eric Sandeen
  2 siblings, 1 reply; 7+ messages in thread
From: Serge E. Hallyn @ 2022-03-01 15:10 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: xfs, Ondrej Mosnacek, Dave Chinner, linux-security-module,
	selinux, john.haxby

On Mon, Feb 28, 2022 at 06:50:52PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> There are a few places where we test the current process' capability set
> to decide if we're going to be more or less generous with resource
> acquisition for a system call.  If the process doesn't have the
> capability, we can continue the call, albeit in a degraded mode.
> 
> These are /not/ the actual security decisions, so it's not proper to use
> capable(), which (in certain selinux setups) causes audit messages to
> get logged.  Switch them to has_capability_noaudit.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Cc: Ondrej Mosnacek <omosnace@redhat.com>
> Cc: Dave Chinner <david@fromorbit.com>
> ---
>  fs/xfs/xfs_fsmap.c  |    4 ++--
>  fs/xfs/xfs_ioctl.c  |    2 +-
>  fs/xfs/xfs_iops.c   |    2 +-
>  kernel/capability.c |    1 +
>  4 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> index 48287caad28b..10e1cb71439e 100644
> --- a/fs/xfs/xfs_fsmap.c
> +++ b/fs/xfs/xfs_fsmap.c
> @@ -864,8 +864,8 @@ xfs_getfsmap(
>  	    !xfs_getfsmap_is_valid_device(mp, &head->fmh_keys[1]))
>  		return -EINVAL;
>  
> -	use_rmap = capable(CAP_SYS_ADMIN) &&
> -		   xfs_has_rmapbt(mp);
> +	use_rmap = xfs_has_rmapbt(mp) &&

Hm, I'm failing to find where xfs_has_rmapbt() is defined.  I just
wanted to make sure it doesn't have any side effects that you'd want
to avoid in the no-capability case.  (Seems very unlikely that it
would, given the name)

> +		   has_capability_noaudit(current, CAP_SYS_ADMIN);
>  	head->fmh_entries = 0;
>  
>  	/* Set up our device handlers. */
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 2515fe8299e1..83481005317a 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1189,7 +1189,7 @@ xfs_ioctl_setattr_get_trans(
>  		goto out_error;
>  
>  	error = xfs_trans_alloc_ichange(ip, NULL, NULL, pdqp,
> -			capable(CAP_FOWNER), &tp);
> +			has_capability_noaudit(current, CAP_FOWNER), &tp);
>  	if (error)
>  		goto out_error;
>  
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index b79b3846e71b..a65217f787cf 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -723,7 +723,7 @@ xfs_setattr_nonsize(
>  	}
>  
>  	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL,
> -			capable(CAP_FOWNER), &tp);
> +			has_capability_noaudit(current, CAP_FOWNER), &tp);
>  	if (error)
>  		goto out_dqrele;
>  
> diff --git a/kernel/capability.c b/kernel/capability.c
> index 46a361dde042..765194f5d678 100644
> --- a/kernel/capability.c
> +++ b/kernel/capability.c
> @@ -360,6 +360,7 @@ bool has_capability_noaudit(struct task_struct *t, int cap)
>  {
>  	return has_ns_capability_noaudit(t, &init_user_ns, cap);
>  }
> +EXPORT_SYMBOL(has_capability_noaudit);
>  
>  static bool ns_capable_common(struct user_namespace *ns,
>  			      int cap,

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RESEND] xfs: don't generate selinux audit messages for capability testing
  2022-03-01 15:10 ` Serge E. Hallyn
@ 2022-03-01 15:48   ` Darrick J. Wong
  2022-03-02 14:44     ` Serge E. Hallyn
  0 siblings, 1 reply; 7+ messages in thread
From: Darrick J. Wong @ 2022-03-01 15:48 UTC (permalink / raw)
  To: Serge E. Hallyn
  Cc: xfs, Ondrej Mosnacek, Dave Chinner, linux-security-module,
	selinux, john.haxby

On Tue, Mar 01, 2022 at 09:10:14AM -0600, Serge E. Hallyn wrote:
> On Mon, Feb 28, 2022 at 06:50:52PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > There are a few places where we test the current process' capability set
> > to decide if we're going to be more or less generous with resource
> > acquisition for a system call.  If the process doesn't have the
> > capability, we can continue the call, albeit in a degraded mode.
> > 
> > These are /not/ the actual security decisions, so it's not proper to use
> > capable(), which (in certain selinux setups) causes audit messages to
> > get logged.  Switch them to has_capability_noaudit.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > Cc: Ondrej Mosnacek <omosnace@redhat.com>
> > Cc: Dave Chinner <david@fromorbit.com>
> > ---
> >  fs/xfs/xfs_fsmap.c  |    4 ++--
> >  fs/xfs/xfs_ioctl.c  |    2 +-
> >  fs/xfs/xfs_iops.c   |    2 +-
> >  kernel/capability.c |    1 +
> >  4 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> > index 48287caad28b..10e1cb71439e 100644
> > --- a/fs/xfs/xfs_fsmap.c
> > +++ b/fs/xfs/xfs_fsmap.c
> > @@ -864,8 +864,8 @@ xfs_getfsmap(
> >  	    !xfs_getfsmap_is_valid_device(mp, &head->fmh_keys[1]))
> >  		return -EINVAL;
> >  
> > -	use_rmap = capable(CAP_SYS_ADMIN) &&
> > -		   xfs_has_rmapbt(mp);
> > +	use_rmap = xfs_has_rmapbt(mp) &&
> 
> Hm, I'm failing to find where xfs_has_rmapbt() is defined.  I just
> wanted to make sure it doesn't have any side effects that you'd want
> to avoid in the no-capability case.  (Seems very unlikely that it
> would, given the name)

fs/xfs/xfs_mount.h:495:__XFS_HAS_FEAT(rmapbt, RMAPBT)

To expand on that a little -- it's a convenience predicate that tells us
whether or not the mounted xfs filesystem supports the reverse mapping
btree feature.  The predicate itself has no side effects, of course, so
the rearranging of the two sides of the && operator so that we do the
cheaper check first (like this code probably should have done from the
start).

--D

> 
> > +		   has_capability_noaudit(current, CAP_SYS_ADMIN);
> >  	head->fmh_entries = 0;
> >  
> >  	/* Set up our device handlers. */
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 2515fe8299e1..83481005317a 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1189,7 +1189,7 @@ xfs_ioctl_setattr_get_trans(
> >  		goto out_error;
> >  
> >  	error = xfs_trans_alloc_ichange(ip, NULL, NULL, pdqp,
> > -			capable(CAP_FOWNER), &tp);
> > +			has_capability_noaudit(current, CAP_FOWNER), &tp);
> >  	if (error)
> >  		goto out_error;
> >  
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index b79b3846e71b..a65217f787cf 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -723,7 +723,7 @@ xfs_setattr_nonsize(
> >  	}
> >  
> >  	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL,
> > -			capable(CAP_FOWNER), &tp);
> > +			has_capability_noaudit(current, CAP_FOWNER), &tp);
> >  	if (error)
> >  		goto out_dqrele;
> >  
> > diff --git a/kernel/capability.c b/kernel/capability.c
> > index 46a361dde042..765194f5d678 100644
> > --- a/kernel/capability.c
> > +++ b/kernel/capability.c
> > @@ -360,6 +360,7 @@ bool has_capability_noaudit(struct task_struct *t, int cap)
> >  {
> >  	return has_ns_capability_noaudit(t, &init_user_ns, cap);
> >  }
> > +EXPORT_SYMBOL(has_capability_noaudit);
> >  
> >  static bool ns_capable_common(struct user_namespace *ns,
> >  			      int cap,

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RESEND] xfs: don't generate selinux audit messages for capability testing
  2022-03-01  2:50 [PATCH RESEND] xfs: don't generate selinux audit messages for capability testing Darrick J. Wong
  2022-03-01 15:10 ` Serge E. Hallyn
@ 2022-03-02 11:01 ` Ondrej Mosnacek
  2022-03-03 17:21 ` Eric Sandeen
  2 siblings, 0 replies; 7+ messages in thread
From: Ondrej Mosnacek @ 2022-03-02 11:01 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: xfs, Dave Chinner, Linux Security Module list, SElinux list, john.haxby

On Tue, Mar 1, 2022 at 3:51 AM Darrick J. Wong <djwong@kernel.org> wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> There are a few places where we test the current process' capability set
> to decide if we're going to be more or less generous with resource
> acquisition for a system call.  If the process doesn't have the
> capability, we can continue the call, albeit in a degraded mode.
>
> These are /not/ the actual security decisions, so it's not proper to use
> capable(), which (in certain selinux setups) causes audit messages to
> get logged.  Switch them to has_capability_noaudit.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Cc: Ondrej Mosnacek <omosnace@redhat.com>
> Cc: Dave Chinner <david@fromorbit.com>
> ---
>  fs/xfs/xfs_fsmap.c  |    4 ++--
>  fs/xfs/xfs_ioctl.c  |    2 +-
>  fs/xfs/xfs_iops.c   |    2 +-
>  kernel/capability.c |    1 +
>  4 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> index 48287caad28b..10e1cb71439e 100644
> --- a/fs/xfs/xfs_fsmap.c
> +++ b/fs/xfs/xfs_fsmap.c
> @@ -864,8 +864,8 @@ xfs_getfsmap(
>             !xfs_getfsmap_is_valid_device(mp, &head->fmh_keys[1]))
>                 return -EINVAL;
>
> -       use_rmap = capable(CAP_SYS_ADMIN) &&
> -                  xfs_has_rmapbt(mp);
> +       use_rmap = xfs_has_rmapbt(mp) &&
> +                  has_capability_noaudit(current, CAP_SYS_ADMIN);
>         head->fmh_entries = 0;
>
>         /* Set up our device handlers. */
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 2515fe8299e1..83481005317a 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1189,7 +1189,7 @@ xfs_ioctl_setattr_get_trans(
>                 goto out_error;
>
>         error = xfs_trans_alloc_ichange(ip, NULL, NULL, pdqp,
> -                       capable(CAP_FOWNER), &tp);
> +                       has_capability_noaudit(current, CAP_FOWNER), &tp);
>         if (error)
>                 goto out_error;
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index b79b3846e71b..a65217f787cf 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -723,7 +723,7 @@ xfs_setattr_nonsize(
>         }
>
>         error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL,
> -                       capable(CAP_FOWNER), &tp);
> +                       has_capability_noaudit(current, CAP_FOWNER), &tp);
>         if (error)
>                 goto out_dqrele;
>
> diff --git a/kernel/capability.c b/kernel/capability.c
> index 46a361dde042..765194f5d678 100644
> --- a/kernel/capability.c
> +++ b/kernel/capability.c
> @@ -360,6 +360,7 @@ bool has_capability_noaudit(struct task_struct *t, int cap)
>  {
>         return has_ns_capability_noaudit(t, &init_user_ns, cap);
>  }
> +EXPORT_SYMBOL(has_capability_noaudit);
>
>  static bool ns_capable_common(struct user_namespace *ns,
>                               int cap,
>

Thank you for respinning the patch[1]! And sorry that I couldn't find
the time to do that :/

Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>

(Yes, we should still clean up and document the capability functions,
but if no one has the energy, let's at least do the minimal fix.)

[1] Link for reference:
https://lore.kernel.org/linux-xfs/20210316173226.2220046-1-omosnace@redhat.com/T/

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RESEND] xfs: don't generate selinux audit messages for capability testing
  2022-03-01 15:48   ` Darrick J. Wong
@ 2022-03-02 14:44     ` Serge E. Hallyn
  0 siblings, 0 replies; 7+ messages in thread
From: Serge E. Hallyn @ 2022-03-02 14:44 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Serge E. Hallyn, xfs, Ondrej Mosnacek, Dave Chinner,
	linux-security-module, selinux, john.haxby

On Tue, Mar 01, 2022 at 07:48:18AM -0800, Darrick J. Wong wrote:
> On Tue, Mar 01, 2022 at 09:10:14AM -0600, Serge E. Hallyn wrote:
> > On Mon, Feb 28, 2022 at 06:50:52PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > There are a few places where we test the current process' capability set
> > > to decide if we're going to be more or less generous with resource
> > > acquisition for a system call.  If the process doesn't have the
> > > capability, we can continue the call, albeit in a degraded mode.
> > > 
> > > These are /not/ the actual security decisions, so it's not proper to use
> > > capable(), which (in certain selinux setups) causes audit messages to
> > > get logged.  Switch them to has_capability_noaudit.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > Cc: Ondrej Mosnacek <omosnace@redhat.com>
> > > Cc: Dave Chinner <david@fromorbit.com>
> > > ---
> > >  fs/xfs/xfs_fsmap.c  |    4 ++--
> > >  fs/xfs/xfs_ioctl.c  |    2 +-
> > >  fs/xfs/xfs_iops.c   |    2 +-
> > >  kernel/capability.c |    1 +
> > >  4 files changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> > > index 48287caad28b..10e1cb71439e 100644
> > > --- a/fs/xfs/xfs_fsmap.c
> > > +++ b/fs/xfs/xfs_fsmap.c
> > > @@ -864,8 +864,8 @@ xfs_getfsmap(
> > >  	    !xfs_getfsmap_is_valid_device(mp, &head->fmh_keys[1]))
> > >  		return -EINVAL;
> > >  
> > > -	use_rmap = capable(CAP_SYS_ADMIN) &&
> > > -		   xfs_has_rmapbt(mp);
> > > +	use_rmap = xfs_has_rmapbt(mp) &&
> > 
> > Hm, I'm failing to find where xfs_has_rmapbt() is defined.  I just
> > wanted to make sure it doesn't have any side effects that you'd want
> > to avoid in the no-capability case.  (Seems very unlikely that it
> > would, given the name)
> 
> fs/xfs/xfs_mount.h:495:__XFS_HAS_FEAT(rmapbt, RMAPBT)
> 
> To expand on that a little -- it's a convenience predicate that tells us
> whether or not the mounted xfs filesystem supports the reverse mapping
> btree feature.  The predicate itself has no side effects, of course, so
> the rearranging of the two sides of the && operator so that we do the
> cheaper check first (like this code probably should have done from the
> start).
> 
> --D

THank you - looks good then.

Acked-by: Serge Hallyn <serge@hallyn.com>

> > 
> > > +		   has_capability_noaudit(current, CAP_SYS_ADMIN);
> > >  	head->fmh_entries = 0;
> > >  
> > >  	/* Set up our device handlers. */
> > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > > index 2515fe8299e1..83481005317a 100644
> > > --- a/fs/xfs/xfs_ioctl.c
> > > +++ b/fs/xfs/xfs_ioctl.c
> > > @@ -1189,7 +1189,7 @@ xfs_ioctl_setattr_get_trans(
> > >  		goto out_error;
> > >  
> > >  	error = xfs_trans_alloc_ichange(ip, NULL, NULL, pdqp,
> > > -			capable(CAP_FOWNER), &tp);
> > > +			has_capability_noaudit(current, CAP_FOWNER), &tp);
> > >  	if (error)
> > >  		goto out_error;
> > >  
> > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > > index b79b3846e71b..a65217f787cf 100644
> > > --- a/fs/xfs/xfs_iops.c
> > > +++ b/fs/xfs/xfs_iops.c
> > > @@ -723,7 +723,7 @@ xfs_setattr_nonsize(
> > >  	}
> > >  
> > >  	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL,
> > > -			capable(CAP_FOWNER), &tp);
> > > +			has_capability_noaudit(current, CAP_FOWNER), &tp);
> > >  	if (error)
> > >  		goto out_dqrele;
> > >  
> > > diff --git a/kernel/capability.c b/kernel/capability.c
> > > index 46a361dde042..765194f5d678 100644
> > > --- a/kernel/capability.c
> > > +++ b/kernel/capability.c
> > > @@ -360,6 +360,7 @@ bool has_capability_noaudit(struct task_struct *t, int cap)
> > >  {
> > >  	return has_ns_capability_noaudit(t, &init_user_ns, cap);
> > >  }
> > > +EXPORT_SYMBOL(has_capability_noaudit);
> > >  
> > >  static bool ns_capable_common(struct user_namespace *ns,
> > >  			      int cap,

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RESEND] xfs: don't generate selinux audit messages for capability testing
  2022-03-01  2:50 [PATCH RESEND] xfs: don't generate selinux audit messages for capability testing Darrick J. Wong
  2022-03-01 15:10 ` Serge E. Hallyn
  2022-03-02 11:01 ` Ondrej Mosnacek
@ 2022-03-03 17:21 ` Eric Sandeen
  2022-03-04  0:30   ` Darrick J. Wong
  2 siblings, 1 reply; 7+ messages in thread
From: Eric Sandeen @ 2022-03-03 17:21 UTC (permalink / raw)
  To: Darrick J. Wong, xfs
  Cc: Ondrej Mosnacek, Dave Chinner, linux-security-module, selinux,
	john.haxby

On 2/28/22 8:50 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> There are a few places where we test the current process' capability set
> to decide if we're going to be more or less generous with resource
> acquisition for a system call.  If the process doesn't have the
> capability, we can continue the call, albeit in a degraded mode.
> 
> These are /not/ the actual security decisions, so it's not proper to use
> capable(), which (in certain selinux setups) causes audit messages to
> get logged.  Switch them to has_capability_noaudit.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Cc: Ondrej Mosnacek <omosnace@redhat.com>
> Cc: Dave Chinner <david@fromorbit.com>

Thanks Darrick. This looks technically correct to me as well.

You might want to add a:

Fixes: 7317a03df703f ("xfs: refactor inode ownership change transaction/inode/quota allocation idiom")

because I /think/ that's the commit that moved the capable() checks out
from under quota tests, and made the problem more visible.

And maybe:

Fixes: ea9a46e1c4925 ("xfs: only return detailed fsmap info if the caller has CAP_SYS_ADMIN")

as well?

It's not strictly fixing the former; AFAICT the problem existed when quota was
enabled already, so I'll leave all that to your discretion.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Thanks,
-Eric

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RESEND] xfs: don't generate selinux audit messages for capability testing
  2022-03-03 17:21 ` Eric Sandeen
@ 2022-03-04  0:30   ` Darrick J. Wong
  0 siblings, 0 replies; 7+ messages in thread
From: Darrick J. Wong @ 2022-03-04  0:30 UTC (permalink / raw)
  To: Eric Sandeen
  Cc: xfs, Ondrej Mosnacek, Dave Chinner, linux-security-module,
	selinux, john.haxby

On Thu, Mar 03, 2022 at 11:21:00AM -0600, Eric Sandeen wrote:
> On 2/28/22 8:50 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > There are a few places where we test the current process' capability set
> > to decide if we're going to be more or less generous with resource
> > acquisition for a system call.  If the process doesn't have the
> > capability, we can continue the call, albeit in a degraded mode.
> > 
> > These are /not/ the actual security decisions, so it's not proper to use
> > capable(), which (in certain selinux setups) causes audit messages to
> > get logged.  Switch them to has_capability_noaudit.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > Cc: Ondrej Mosnacek <omosnace@redhat.com>
> > Cc: Dave Chinner <david@fromorbit.com>
> 
> Thanks Darrick. This looks technically correct to me as well.
> 
> You might want to add a:
> 
> Fixes: 7317a03df703f ("xfs: refactor inode ownership change transaction/inode/quota allocation idiom")
> 
> because I /think/ that's the commit that moved the capable() checks out
> from under quota tests, and made the problem more visible.
> 
> And maybe:
> 
> Fixes: ea9a46e1c4925 ("xfs: only return detailed fsmap info if the caller has CAP_SYS_ADMIN")
> 
> as well?
> 
> It's not strictly fixing the former; AFAICT the problem existed when quota was
> enabled already, so I'll leave all that to your discretion.
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Thank you.

--D

> Thanks,
> -Eric

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-03-04  0:30 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-01  2:50 [PATCH RESEND] xfs: don't generate selinux audit messages for capability testing Darrick J. Wong
2022-03-01 15:10 ` Serge E. Hallyn
2022-03-01 15:48   ` Darrick J. Wong
2022-03-02 14:44     ` Serge E. Hallyn
2022-03-02 11:01 ` Ondrej Mosnacek
2022-03-03 17:21 ` Eric Sandeen
2022-03-04  0:30   ` Darrick J. Wong

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).