[PATCH 0/2] Fix capabilities check

[PATCH 0/2] Fix capabilities check

[cem]

From: Carlos Maiolino <cem@kernel.org>

First patch fixes the original replacement of capable() calls in a
straight-forward way which can be backported to stable kernels in an
easy way.

Second patch lifts the capability check into xfs_trans_dqresv Christoph
suggested.

I'm working on the capabilities code to see if a helper is worth there
too. But it's worth quickly fixing this in xfs before having all the
bits in capability.c worked out.
There are also a couple other places outside xfs which likely needs
fixing. I'll include it on the next series non-xfs related.

Fixes: eba0549bc7d1 ("xfs: don't generate selinux audit messages for capability testing")
Cc: <stable@vger.kernel.org> # v5.18
Cc: Darrick J. Wong <djwong@kernel.org>
Cc: Eric Sandeen <sandeen@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Jan Kara <jack@suse.cz>
Cc: Dave Chinner <david@fromorbit.com>
Reported-by: Dr. Thomas Orgis <thomas.orgis@uni-hamburg.de>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_fsmap.c | 2 +-
 fs/xfs/xfs_ioctl.c | 2 +-
 fs/xfs/xfs_iops.c  | 3 ++-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
index b6a3bc9f143c..7c79fbe0a74c 100644
--- a/fs/xfs/xfs_fsmap.c
+++ b/fs/xfs/xfs_fsmap.c
@@ -1175,7 +1175,7 @@ xfs_getfsmap(
 		return -EINVAL;
 
 	use_rmap = xfs_has_rmapbt(mp) &&
-		   has_capability_noaudit(current, CAP_SYS_ADMIN);
+		   ns_capable_noaudit(&init_user_ns, 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 96af6b62ce39..852ff2ab4531 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -647,7 +647,7 @@ xfs_ioctl_setattr_get_trans(
 		goto out_error;
 
 	error = xfs_trans_alloc_ichange(ip, NULL, NULL, pdqp,
-			has_capability_noaudit(current, CAP_FOWNER), &tp);
+			ns_capable_noaudit(&init_user_ns, CAP_FOWNER), &tp);
 	if (error)
 		goto out_error;
 
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 325c2200c501..9db9ef1d8c3a 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -835,7 +835,8 @@ xfs_setattr_nonsize(
 	}
 
 	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL,
-			has_capability_noaudit(current, CAP_FOWNER), &tp);
+				ns_capable_noaudit(&init_user_ns, CAP_FOWNER),
+				&tp);
 	if (error)
 		goto out_dqrele;
 
-- 
2.54.0


Carlos Maiolino (2):
  xfs: fix capabily check in xfs
  xfs: lift quota capability check to xfs_trans_dqresv

 fs/xfs/xfs_fsmap.c       | 2 +-
 fs/xfs/xfs_ioctl.c       | 3 +--
 fs/xfs/xfs_iops.c        | 3 +--
 fs/xfs/xfs_trans_dquot.c | 6 ++++--
 4 files changed, 7 insertions(+), 7 deletions(-)

-- 
2.54.0

[PATCH 1/2] xfs: fix capabily check in xfs

[cem]

From: Carlos Maiolino <cem@kernel.org>

An user reported a bug where he managed to evade group's quota
by changing a file's gid to a different group id the same user
belonged to, even though quotas were enforced on both gids and the
file's size was big enough to exceed the quota's hardlimit.

Commit eba0549bc7d1 replaced a capable() call by a
has_capability_noaudit() to prevent unnecessary selinux audit messages.
Turns out that both calls have slightly different semantics even though
their documentation seems similar. Where in a nutshell:

capable() - Tests the task's effective credentials
has_ns_capability_noaudit() - Tests the task's real credentials

This most of the time has no practical difference but in some cases like
changing attrs (specifically group id in this case) through a NFS client
this will allow the quota code to use XFS_QMOPT_FORCE_RES, effectively
bypassing quota accounting checks.

Using instead ns_capable_noaudit() should fix this issue and prevent
selinux audit messages.

This also fix the remaining calls to has_capability_noaudit()

Fixes: eba0549bc7d1 ("xfs: don't generate selinux audit messages for capability testing")
Cc: <stable@vger.kernel.org> # v5.18
Cc: Darrick J. Wong <djwong@kernel.org>
Cc: Eric Sandeen <sandeen@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Jan Kara <jack@suse.cz>
Cc: Dave Chinner <david@fromorbit.com>
Reported-by: Dr. Thomas Orgis <thomas.orgis@uni-hamburg.de>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_fsmap.c | 2 +-
 fs/xfs/xfs_ioctl.c | 2 +-
 fs/xfs/xfs_iops.c  | 3 ++-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
index b6a3bc9f143c..7c79fbe0a74c 100644
--- a/fs/xfs/xfs_fsmap.c
+++ b/fs/xfs/xfs_fsmap.c
@@ -1175,7 +1175,7 @@ xfs_getfsmap(
 		return -EINVAL;
 
 	use_rmap = xfs_has_rmapbt(mp) &&
-		   has_capability_noaudit(current, CAP_SYS_ADMIN);
+		   ns_capable_noaudit(&init_user_ns, 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 96af6b62ce39..852ff2ab4531 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -647,7 +647,7 @@ xfs_ioctl_setattr_get_trans(
 		goto out_error;
 
 	error = xfs_trans_alloc_ichange(ip, NULL, NULL, pdqp,
-			has_capability_noaudit(current, CAP_FOWNER), &tp);
+			ns_capable_noaudit(&init_user_ns, CAP_FOWNER), &tp);
 	if (error)
 		goto out_error;
 
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 325c2200c501..9db9ef1d8c3a 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -835,7 +835,8 @@ xfs_setattr_nonsize(
 	}
 
 	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL,
-			has_capability_noaudit(current, CAP_FOWNER), &tp);
+				ns_capable_noaudit(&init_user_ns, CAP_FOWNER),
+				&tp);
 	if (error)
 		goto out_dqrele;
 
-- 
2.54.0

[PATCH 2/2] xfs: lift quota capability check to xfs_trans_dqresv

[cem]

From: Carlos Maiolino <cem@kernel.org>

Instead of calling into ns_capable_noaudit() as an argument to
xfs_trans_alloc_ichange(), just pass false where XFS_QMOPT_FORCE_RES is
not explicitly required and let xfs_trans_dqresv() decide if quotas
should be bypassed or not.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_ioctl.c       | 3 +--
 fs/xfs/xfs_iops.c        | 4 +---
 fs/xfs/xfs_trans_dquot.c | 6 ++++--
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 852ff2ab4531..480feddc0e51 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -646,8 +646,7 @@ xfs_ioctl_setattr_get_trans(
 	if (xfs_is_shutdown(mp))
 		goto out_error;
 
-	error = xfs_trans_alloc_ichange(ip, NULL, NULL, pdqp,
-			ns_capable_noaudit(&init_user_ns, CAP_FOWNER), &tp);
+	error = xfs_trans_alloc_ichange(ip, NULL, NULL, pdqp, false, &tp);
 	if (error)
 		goto out_error;
 
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 9db9ef1d8c3a..d2ae682f749f 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -834,9 +834,7 @@ xfs_setattr_nonsize(
 			return error;
 	}
 
-	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL,
-				ns_capable_noaudit(&init_user_ns, CAP_FOWNER),
-				&tp);
+	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL, false, &tp);
 	if (error)
 		goto out_dqrele;
 
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index eaf9de6e07fd..50e5b323f7f1 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -832,8 +832,10 @@ xfs_trans_dqresv(
 		qlim = &defq->rtb;
 	}
 
-	if ((flags & XFS_QMOPT_FORCE_RES) == 0 && dqp->q_id &&
-	    xfs_dquot_is_enforced(dqp)) {
+	if ((flags & XFS_QMOPT_FORCE_RES) == 0 &&
+	    dqp->q_id &&
+	    xfs_dquot_is_enforced(dqp) &&
+	    !ns_capable_noaudit(&init_user_ns, CAP_SYS_RESOURCE)) {
 		int		quota_nl;
 		bool		fatal;
 
-- 
2.54.0

Re: [PATCH 1/2] xfs: fix capabily check in xfs

[Darrick J. Wong]

Note: s/capabily/capability/ in the subject line

On Fri, Jun 26, 2026 at 12:29:24PM +0200, cem@kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
> 
> An user reported a bug where he managed to evade group's quota
> by changing a file's gid to a different group id the same user
> belonged to, even though quotas were enforced on both gids and the
> file's size was big enough to exceed the quota's hardlimit.
> 
> Commit eba0549bc7d1 replaced a capable() call by a
> has_capability_noaudit() to prevent unnecessary selinux audit messages.
> Turns out that both calls have slightly different semantics even though
> their documentation seems similar. Where in a nutshell:
> 
> capable() - Tests the task's effective credentials
> has_ns_capability_noaudit() - Tests the task's real credentials
> 
> This most of the time has no practical difference but in some cases like
> changing attrs (specifically group id in this case) through a NFS client
> this will allow the quota code to use XFS_QMOPT_FORCE_RES, effectively
> bypassing quota accounting checks.
> 
> Using instead ns_capable_noaudit() should fix this issue and prevent
> selinux audit messages.
> 
> This also fix the remaining calls to has_capability_noaudit()
> 
> Fixes: eba0549bc7d1 ("xfs: don't generate selinux audit messages for capability testing")
> Cc: <stable@vger.kernel.org> # v5.18
> Cc: Darrick J. Wong <djwong@kernel.org>
> Cc: Eric Sandeen <sandeen@redhat.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Dave Chinner <david@fromorbit.com>
> Reported-by: Dr. Thomas Orgis <thomas.orgis@uni-hamburg.de>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_fsmap.c | 2 +-
>  fs/xfs/xfs_ioctl.c | 2 +-
>  fs/xfs/xfs_iops.c  | 3 ++-
>  3 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> index b6a3bc9f143c..7c79fbe0a74c 100644
> --- a/fs/xfs/xfs_fsmap.c
> +++ b/fs/xfs/xfs_fsmap.c
> @@ -1175,7 +1175,7 @@ xfs_getfsmap(
>  		return -EINVAL;
>  
>  	use_rmap = xfs_has_rmapbt(mp) &&
> -		   has_capability_noaudit(current, CAP_SYS_ADMIN);
> +		   ns_capable_noaudit(&init_user_ns, 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 96af6b62ce39..852ff2ab4531 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -647,7 +647,7 @@ xfs_ioctl_setattr_get_trans(
>  		goto out_error;
>  
>  	error = xfs_trans_alloc_ichange(ip, NULL, NULL, pdqp,
> -			has_capability_noaudit(current, CAP_FOWNER), &tp);
> +			ns_capable_noaudit(&init_user_ns, CAP_FOWNER), &tp);
>  	if (error)
>  		goto out_error;
>  
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 325c2200c501..9db9ef1d8c3a 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -835,7 +835,8 @@ xfs_setattr_nonsize(
>  	}
>  
>  	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL,
> -			has_capability_noaudit(current, CAP_FOWNER), &tp);
> +				ns_capable_noaudit(&init_user_ns, CAP_FOWNER),
> +				&tp);

Extra indenting of the second and third lines, but otherwise this looks
good to me.  With the indent fixed,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D


>  	if (error)
>  		goto out_dqrele;
>  
> -- 
> 2.54.0
> 
> 

Re: [PATCH 2/2] xfs: lift quota capability check to xfs_trans_dqresv

[Darrick J. Wong]

On Fri, Jun 26, 2026 at 12:29:25PM +0200, cem@kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
> 
> Instead of calling into ns_capable_noaudit() as an argument to
> xfs_trans_alloc_ichange(), just pass false where XFS_QMOPT_FORCE_RES is
> not explicitly required and let xfs_trans_dqresv() decide if quotas
> should be bypassed or not.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_ioctl.c       | 3 +--
>  fs/xfs/xfs_iops.c        | 4 +---
>  fs/xfs/xfs_trans_dquot.c | 6 ++++--
>  3 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 852ff2ab4531..480feddc0e51 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -646,8 +646,7 @@ xfs_ioctl_setattr_get_trans(
>  	if (xfs_is_shutdown(mp))
>  		goto out_error;
>  
> -	error = xfs_trans_alloc_ichange(ip, NULL, NULL, pdqp,
> -			ns_capable_noaudit(&init_user_ns, CAP_FOWNER), &tp);
> +	error = xfs_trans_alloc_ichange(ip, NULL, NULL, pdqp, false, &tp);
>  	if (error)
>  		goto out_error;
>  
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 9db9ef1d8c3a..d2ae682f749f 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -834,9 +834,7 @@ xfs_setattr_nonsize(
>  			return error;
>  	}
>  
> -	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL,
> -				ns_capable_noaudit(&init_user_ns, CAP_FOWNER),
> -				&tp);
> +	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL, false, &tp);

Hrm.  On second viewing, I don't really like burying the capability
check down in the transaction code because this overrides the higher
level logic that sets (or doesn't set) XFS_QMOPT_FORCE_RES.

>  	if (error)
>  		goto out_dqrele;
>  
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index eaf9de6e07fd..50e5b323f7f1 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -832,8 +832,10 @@ xfs_trans_dqresv(
>  		qlim = &defq->rtb;
>  	}
>  
> -	if ((flags & XFS_QMOPT_FORCE_RES) == 0 && dqp->q_id &&
> -	    xfs_dquot_is_enforced(dqp)) {
> +	if ((flags & XFS_QMOPT_FORCE_RES) == 0 &&
> +	    dqp->q_id &&
> +	    xfs_dquot_is_enforced(dqp) &&
> +	    !ns_capable_noaudit(&init_user_ns, CAP_SYS_RESOURCE)) {

The other thing that bugs me is changing CAP_FOWNER to CAP_SYS_RESOURCE
-- that's not mentioned in the commit message even though it's a
potentially breaking change for any program that already internalized
doing that on only XFS.  At a minimum we'd also have to change the
xfs_scrub service files to have CAP_SYS_RESOURCE (in addition to FOWNER)
avoid EDQUOT during online repairs.

Looking back in the history of XFS and VFS quotas, I guess XFS has been
doing CAP_FOWNER since 2.4.25, and VFS has been doing CAP_SYS_RESOURCE
since 2.3.30.  So XFS was the one that deviated (possibly because FOWNER
was the established privilege on Irix?) but I think that means XFS has
to accept either.

Going back to my idea from yesterday, I now amend it to:

static inline bool
current_may_ignore_quota_limits(void)
{
	/*
	 * Linux VFS quota code allows ignoring hard limits on
	 * chown/chgrp if the effective credentials include
	 * CAP_SYS_RESOURCE.
	 */
	if (ns_capable_noaudit(&init_user_ns, CAP_SYS_RESOURCE))
		return true;

	/*
	 * Historically, XFS also allows that if the current process'
	 * effective credentials include CAP_FOWNER, then they're
	 * allowed to ignore the hard limit.
	 */
	return ns_capable_noaudit(&init_user_ns, CAP_FOWNER);
}

The callsites are still:

	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL,
			current_may_ignore_quota_limits(), &tp);

--D

>  		int		quota_nl;
>  		bool		fatal;
>  
> -- 
> 2.54.0
> 
>