Skip to contentSkip to conversation

Fix capabilities check

PATCH Superseded [email protected] 2 participants 6 messages Jun 26 – Jun 29

Subsystems (2)
Stable branch XFS filesystem

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

cover Fix capabilities check

From: Carlos Maiolino <[email protected]>

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 <[email protected]> # v5.18
Cc Darrick J. Wong <[email protected]>
Cc Eric Sandeen <[email protected]>
Cc Christoph Hellwig <[email protected]>
Cc Jan Kara <[email protected]>
Cc Dave Chinner <[email protected]>
Reported-by Dr. Thomas Orgis <[email protected]>
Signed-off-by Carlos Maiolino <[email protected]>
---
 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(-)
fs/xfs/xfs_fsmap.c +1 -1
index b6a3bc9f143c..7c79fbe0a74c 100644
@@ -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. */
fs/xfs/xfs_ioctl.c +1 -1
index 96af6b62ce39..852ff2ab4531 100644
@@ -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;
fs/xfs/xfs_iops.c +2 -1
index 325c2200c501..9db9ef1d8c3a 100644
@@ -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;
Signature · 15 lines
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
author maintainer
#1 lore raw

PATCH 1/2 xfs: fix capabily check in xfs

From: Carlos Maiolino <[email protected]>

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 <[email protected]> # v5.18
Cc Darrick J. Wong <[email protected]>
Cc Eric Sandeen <[email protected]>
Cc Christoph Hellwig <[email protected]>
Cc Jan Kara <[email protected]>
Cc Dave Chinner <[email protected]>
Reported-by Dr. Thomas Orgis <[email protected]>
Signed-off-by Carlos Maiolino <[email protected]>
---
 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(-)
fs/xfs/xfs_fsmap.c +1 -1
index b6a3bc9f143c..7c79fbe0a74c 100644
@@ -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. */
fs/xfs/xfs_ioctl.c +1 -1
index 96af6b62ce39..852ff2ab4531 100644
@@ -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;
fs/xfs/xfs_iops.c +2 -1
index 325c2200c501..9db9ef1d8c3a 100644
@@ -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;
Signature · 1 line
2.54.0
Reviewed-by on v1 1/2
#2 lore raw

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

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

Quoted 71 lines · On Fri, Jun 26, 2026 at 12:29:24PM +0200, [email protected] wrote:

From: Carlos Maiolino <[email protected]>

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 <[email protected]> # v5.18
Cc Darrick J. Wong <[email protected]>
Cc Eric Sandeen <[email protected]>
Cc Christoph Hellwig <[email protected]>
Cc Jan Kara <[email protected]>
Cc Dave Chinner <[email protected]>
Reported-by Dr. Thomas Orgis <[email protected]>
Signed-off-by Carlos Maiolino <[email protected]>
---
 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(-)
fs/xfs/xfs_fsmap.c +1 -1
index b6a3bc9f143c..7c79fbe0a74c 100644
@@ -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. */
fs/xfs/xfs_ioctl.c +1 -1
index 96af6b62ce39..852ff2ab4531 100644
@@ -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;
fs/xfs/xfs_iops.c +0 -0
index 325c2200c501..9db9ef1d8c3a 100644

fs/xfs/xfs_iops.c xfs_setattr_nonsize( · line 839

Quoted 6 lines
fs/xfs/xfs_iops.c +2 -1
@@ -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" <[email protected]>

--D

Quoted 7 lines
fs/xfs/xfs_iops.c +0 -0
 	if (error)
 		goto out_dqrele;
Signature · 1 line
2.54.0
author maintainer
#3 lore raw

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

In reply to [email protected]

From: Carlos Maiolino <[email protected]>

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 <[email protected]>
---
 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(-)
fs/xfs/xfs_ioctl.c +1 -2
index 852ff2ab4531..480feddc0e51 100644
@@ -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;
fs/xfs/xfs_iops.c +1 -3
index 9db9ef1d8c3a..d2ae682f749f 100644
@@ -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;
fs/xfs/xfs_trans_dquot.c +4 -2
index eaf9de6e07fd..50e5b323f7f1 100644
@@ -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;
Signature · 1 line
2.54.0
#4 lore raw

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

Quoted 33 lines · On Fri, Jun 26, 2026 at 12:29:25PM +0200, [email protected] wrote:

From: Carlos Maiolino <[email protected]>

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 <[email protected]>
---
 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(-)
fs/xfs/xfs_ioctl.c +1 -2
index 852ff2ab4531..480feddc0e51 100644
@@ -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;
fs/xfs/xfs_iops.c +0 -0
index 9db9ef1d8c3a..d2ae682f749f 100644

fs/xfs/xfs_iops.c xfs_setattr_nonsize( · line 837

Quoted 7 lines
fs/xfs/xfs_iops.c +1 -3
@@ -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.

Quoted 8 lines
fs/xfs/xfs_iops.c +0 -0
 	if (error)
 		goto out_dqrele;
fs/xfs/xfs_trans_dquot.c +0 -0
index eaf9de6e07fd..50e5b323f7f1 100644

fs/xfs/xfs_trans_dquot.c xfs_trans_dqresv( · line 838

Quoted 9 lines
fs/xfs/xfs_trans_dquot.c +4 -2
@@ -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

Quoted 7 lines
fs/xfs/xfs_trans_dquot.c +0 -0
 		int		quota_nl;
 		bool		fatal;
Signature · 1 line
2.54.0
author maintainer
#5 lore raw

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

Quoted 37 lines · Darrick J. Wong
Quoted 36 lines · On Fri, Jun 26, 2026 at 12:29:25PM +0200, [email protected] wrote:

From: Carlos Maiolino <[email protected]>

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 <[email protected]>
---
 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(-)
fs/xfs/xfs_ioctl.c +1 -2
index 852ff2ab4531..480feddc0e51 100644
@@ -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;
fs/xfs/xfs_iops.c +0 -0
index 9db9ef1d8c3a..d2ae682f749f 100644
@@ -834,9 +834,7 @@ xfs_setattr_nonsize(
 			return error;
 	}
Quoted 8 lines

fs/xfs/xfs_iops.c xfs_setattr_nonsize( · line 837

Quoted 4 lines
fs/xfs/xfs_iops.c +1 -3
-	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.

Ok, with a fresh look at it I tend to agree with you here. I'd rather not bury it into the transaction code either. Let's wait and see if hch is ok if we keep this on the caller.

Quoted 12 lines
Quoted 12 lines
fs/xfs/xfs_iops.c +0 -0
 	if (error)
 		goto out_dqrele;
fs/xfs/xfs_trans_dquot.c +0 -0
index eaf9de6e07fd..50e5b323f7f1 100644
@@ -832,8 +832,10 @@ xfs_trans_dqresv(
 		qlim = &defq->rtb;
 	}
Quoted 13 lines

fs/xfs/xfs_trans_dquot.c xfs_trans_dqresv( · line 838

Quoted 6 lines
fs/xfs/xfs_trans_dquot.c +4 -2
-	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.

This is a mistake from my part, after sending it I realized I shouldn't have changed to CAP_SYS_RESOURCE, next version I'll stick with CAP_FOWNER.

Quoted 6 lines

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.

Yup, we do use CAP_FOWNER since forever. I agree we sould keep it.

Quoted 5 lines

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

static inline bool
current_may_ignore_quota_limits(void)

Shorter name perhaps? :)

Quoted 32 lines

{
/*
* 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

Quoted 7 lines
fs/xfs/xfs_trans_dquot.c +0 -0
 		int		quota_nl;
 		bool		fatal;
Signature · 1 line
2.54.0