[PATCH] xfs: fix capabily check in xfs_setattr_nonsize

[PATCH] xfs: fix capabily check in xfs_setattr_nonsize

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

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: Dave Chinner <david@fromorbit.com>
Cc: Eric Sandeen <sandeen@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Reported-by: Dr. Thomas Orgis <thomas.orgis@uni-hamburg.de>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
Signed-off-by: Carlos Maiolino <cem@kernel.org>
---

If people do agree with the fix I do plan to send a patch to
kernel/capability.c to ad a new capable_noaudit() helper which would
come in handy here, but I believe we should backport this all the way
back to 5.18 and replacing it by ns_capable_noaudit() is the easiest way
to do it. Then if capable_noaudit() is acceptable we could just call
this instead. This should also be a test in xfstests.

The patch is still running on my testing suite, I'm sending it ahead of
having the testing finished for discussion/review, so for now it fixes
the problem but I am not sure it doesn't break anything else :)

 fs/xfs/xfs_iops.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 325c2200c501..df0eba26dda3 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -792,6 +792,8 @@ xfs_setattr_nonsize(
 	kgid_t			gid = GLOBAL_ROOT_GID;
 	struct xfs_dquot	*udqp = NULL, *gdqp = NULL;
 	struct xfs_dquot	*old_udqp = NULL, *old_gdqp = NULL;
+	bool			force = ns_capable_noaudit(&init_user_ns,
+							   CAP_FOWNER);
 
 	ASSERT((mask & ATTR_SIZE) == 0);
 
@@ -835,7 +837,7 @@ xfs_setattr_nonsize(
 	}
 
 	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL,
-			has_capability_noaudit(current, CAP_FOWNER), &tp);
+			force, &tp);
 	if (error)
 		goto out_dqrele;
 
-- 
2.54.0

Re: [PATCH] xfs: fix capabily check in xfs_setattr_nonsize

[Christoph Hellwig]

Adding Jan and Christian for quota and user_ns knowledge.

On Wed, Jun 24, 2026 at 12:14:29PM +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

Eww..

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

Yeah, this does look wrong.  Do the other conversion in the above commit
have tthe same issue?

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

The generic quota code manages to do without either has_capability_noaudit
or ns_capable_noaudit.  I think this might be hidden behind
inode_owner_or_capable calls.  Any idea why we're different?

> +	bool			force = ns_capable_noaudit(&init_user_ns,
> +							   CAP_FOWNER);

I'd do away with the local variable here.

Re: [PATCH] xfs: fix capabily check in xfs_setattr_nonsize

[Carlos Maiolino]

On Wed, Jun 24, 2026 at 03:40:39PM +0200, Christoph Hellwig wrote:
> Adding Jan and Christian for quota and user_ns knowledge.
> 
> On Wed, Jun 24, 2026 at 12:14:29PM +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
> 
> Eww..
> 
> > 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.
> 
> Yeah, this does look wrong.  Do the other conversion in the above commit
> have tthe same issue?

I don't know yet, but I do hope to be able to convert everything back to
a capable_noaudit() helper. I need some time to look at the other
conversions, but giving the different credentials likely the are all
victim of the same issue.

> 
> > Using instead ns_capable_noaudit() should fix this issue and prevent
> > selinux audit messages.
> 
> The generic quota code manages to do without either has_capability_noaudit
> or ns_capable_noaudit.  I think this might be hidden behind
> inode_owner_or_capable calls.  Any idea why we're different?

Yes, I looked into that to check the feasibility to use the generic
code. This specific path goes through __dquot_transfer() which uses
ignore_hardlimit() helper to decide between bypassing quota enforcement
or not.
The later though uses capable(CAP_SYS_RESOURCE) which would trigger
selinux audit messages too at least for our case.

This though made me wonder if we shouldn't be using CAP_SYS_RESOURCE in
lieu of CAP_FOWNER in this path which seems more appropriate to bypass
quota enforcement. Either way though, I think we should at least have an
initial simple to backport patch to older releases.


> 
> > +	bool			force = ns_capable_noaudit(&init_user_ns,
> > +							   CAP_FOWNER);
> 
> I'd do away with the local variable here.

Sounds find to me. I wanted to avoid the function call with two arguments
as an argument. But I honestly have no strong preference.

Cheers!

Re: [PATCH] xfs: fix capabily check in xfs_setattr_nonsize

[Jan Kara]

On Wed 24-06-26 15:40:39, Christoph Hellwig wrote:
> Adding Jan and Christian for quota and user_ns knowledge.
> 
> On Wed, Jun 24, 2026 at 12:14:29PM +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
> 
> Eww..

Yeah, that's a catch.

> > 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.
> 
> Yeah, this does look wrong.  Do the other conversion in the above commit
> have tthe same issue?
> 
> > Using instead ns_capable_noaudit() should fix this issue and prevent
> > selinux audit messages.
> 
> The generic quota code manages to do without either has_capability_noaudit
> or ns_capable_noaudit.  I think this might be hidden behind
> inode_owner_or_capable calls.  Any idea why we're different?

Actually no. Generic quota code has equivalent checks in ignore_hardlimit()
function which does capable(CAP_SYS_RESOURCE) check. I guess the reason why
nobody complained about generic quota code is that we call
ignore_hardlimit() only if we are above hardlimit whereas XFS calls this
for every transaction...

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] xfs: fix capabily check in xfs_setattr_nonsize

[Carlos Maiolino]

On Thu, Jun 25, 2026 at 12:43:54PM +0200, Jan Kara wrote:
> On Wed 24-06-26 15:40:39, Christoph Hellwig wrote:
> > Adding Jan and Christian for quota and user_ns knowledge.
> > 
> > On Wed, Jun 24, 2026 at 12:14:29PM +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
> > 
> > Eww..
> 
> Yeah, that's a catch.
> 
> > > 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.
> > 
> > Yeah, this does look wrong.  Do the other conversion in the above commit
> > have tthe same issue?
> > 
> > > Using instead ns_capable_noaudit() should fix this issue and prevent
> > > selinux audit messages.
> > 
> > The generic quota code manages to do without either has_capability_noaudit
> > or ns_capable_noaudit.  I think this might be hidden behind
> > inode_owner_or_capable calls.  Any idea why we're different?
> 
> Actually no. Generic quota code has equivalent checks in ignore_hardlimit()
> function which does capable(CAP_SYS_RESOURCE) check. I guess the reason why
> nobody complained about generic quota code is that we call
> ignore_hardlimit() only if we are above hardlimit whereas XFS calls this
> for every transaction...

But the capable() call works fine, it's the replacement by
has_capability_noaudit() that enables quota bypass I *think* generic
quota code is safe from this point.

> 
> 								Honza
> 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Re: [PATCH] xfs: fix capabily check in xfs_setattr_nonsize

[Christoph Hellwig]

On Thu, Jun 25, 2026 at 12:43:54PM +0200, Jan Kara wrote:
> On Wed 24-06-26 15:40:39, Christoph Hellwig wrote:
> > Adding Jan and Christian for quota and user_ns knowledge.
> > 
> > On Wed, Jun 24, 2026 at 12:14:29PM +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
> > 
> > Eww..
> 
> Yeah, that's a catch.
> 
> > > 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.
> > 
> > Yeah, this does look wrong.  Do the other conversion in the above commit
> > have tthe same issue?
> > 
> > > Using instead ns_capable_noaudit() should fix this issue and prevent
> > > selinux audit messages.
> > 
> > The generic quota code manages to do without either has_capability_noaudit
> > or ns_capable_noaudit.  I think this might be hidden behind
> > inode_owner_or_capable calls.  Any idea why we're different?
> 
> Actually no. Generic quota code has equivalent checks in ignore_hardlimit()
> function which does capable(CAP_SYS_RESOURCE) check. I guess the reason why
> nobody complained about generic quota code is that we call
> ignore_hardlimit() only if we are above hardlimit whereas XFS calls this
> for every transaction...

I guess we should aim for the same to avoid the spurious audit logs.

I.e. xfs_trans_alloc_ichange is currently always called either with
force = true or force = this capable check.  So as a first step we can
move the check into xfs_trans_alloc_ichange for the !force case, and the
propagate that through XFS_QMOPT_FORCE_RES into xfs_trans_dqresv, i.e.
only set XFS_QMOPT_FORCE_RES for the real forced case and instead
have the capable check down in xfs_trans_dqresv.

Re: [PATCH] xfs: fix capabily check in xfs_setattr_nonsize

[Carlos Maiolino]

On Thu, Jun 25, 2026 at 02:37:54PM +0200, Christoph Hellwig wrote:
> On Thu, Jun 25, 2026 at 12:43:54PM +0200, Jan Kara wrote:
> > On Wed 24-06-26 15:40:39, Christoph Hellwig wrote:
> > > Adding Jan and Christian for quota and user_ns knowledge.
> > > 
> > > On Wed, Jun 24, 2026 at 12:14:29PM +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
> > > 
> > > Eww..
> > 
> > Yeah, that's a catch.
> > 
> > > > 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.
> > > 
> > > Yeah, this does look wrong.  Do the other conversion in the above commit
> > > have tthe same issue?
> > > 
> > > > Using instead ns_capable_noaudit() should fix this issue and prevent
> > > > selinux audit messages.
> > > 
> > > The generic quota code manages to do without either has_capability_noaudit
> > > or ns_capable_noaudit.  I think this might be hidden behind
> > > inode_owner_or_capable calls.  Any idea why we're different?
> > 
> > Actually no. Generic quota code has equivalent checks in ignore_hardlimit()
> > function which does capable(CAP_SYS_RESOURCE) check. I guess the reason why
> > nobody complained about generic quota code is that we call
> > ignore_hardlimit() only if we are above hardlimit whereas XFS calls this
> > for every transaction...
> 
> I guess we should aim for the same to avoid the spurious audit logs.
> 
> I.e. xfs_trans_alloc_ichange is currently always called either with
> force = true or force = this capable check.  So as a first step we can
> move the check into xfs_trans_alloc_ichange for the !force case, and the
> propagate that through XFS_QMOPT_FORCE_RES into xfs_trans_dqresv, i.e.
> only set XFS_QMOPT_FORCE_RES for the real forced case and instead
> have the capable check down in xfs_trans_dqresv.
> 

Sounds fair, I'll give it a try tomorrow.

Re: [PATCH] xfs: fix capabily check in xfs_setattr_nonsize

[Darrick J. Wong]

On Thu, Jun 25, 2026 at 05:00:21PM +0200, Carlos Maiolino wrote:
> On Thu, Jun 25, 2026 at 02:37:54PM +0200, Christoph Hellwig wrote:
> > On Thu, Jun 25, 2026 at 12:43:54PM +0200, Jan Kara wrote:
> > > On Wed 24-06-26 15:40:39, Christoph Hellwig wrote:
> > > > Adding Jan and Christian for quota and user_ns knowledge.
> > > > 
> > > > On Wed, Jun 24, 2026 at 12:14:29PM +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
> > > > 
> > > > Eww..
> > > 
> > > Yeah, that's a catch.

I spent a while trying to figure out how I went wrong in selecting the
function name:

 * capable - Determine if the current task has a superior capability in effect
 * @cap: The capability to be tested for
 *
 * Return true if the current task has the given superior capability currently
 * available for use, false if not.

vs.

 * has_capability_noaudit - Does a task have a capability (unaudited) in the
 * initial user ns
 * @t: The task in question
 * @cap: The capability to be tested for
 *
 * Return true if the specified task has the given superior capability
 * currently in effect to init_user_ns, false if not.  Don't write an
 * audit message for the check.

vs.

 * ns_capable_noaudit - Determine if the current task has a superior capability
 * (unaudited) in effect
 * @ns:  The usernamespace we want the capability in
 * @cap: The capability to be tested for
 *
 * Return true if the current task has the given superior capability currently
 * available for use, false if not.

All three of these sound the same to me.  But what about the call path?

capable -> ns_capable -> ns_capable_common

has_capability_noaudit -> has_ns_capability_noaudit

ns_capable_noaudit -> ns_capable_common

Hum, maybe that's the difference -- ns_capable_common vs.
has_ns_capability_noaudit?

ns_capable_common calls security_capable() with current_cred(), whereas
has_ns_capability_noaudit calls it with __task_cred(t), which is
@current in the xfs_trans_alloc_ichange case.

Aha!  current_cred is current->cred, whereas __task_cred(current) is
current->real_cred, and these aren't the same thing:

	/* Objective and real subjective task credentials (COW): */
	const struct cred __rcu		*real_cred;

	/* Effective (overridable) subjective task credentials (COW): */
	const struct cred __rcu		*cred;

So I guess ns_capable_common (and wrappers) are testing the process'
effective credentials, whereas has_ns_capability_noaudit is testing the
process' real credentials?

It would have been *really* nice if the documentation for those
functions mentioned that distinction!

> > > > > 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.
> > > > 
> > > > Yeah, this does look wrong.  Do the other conversion in the above commit
> > > > have tthe same issue?
> > > > 
> > > > > Using instead ns_capable_noaudit() should fix this issue and prevent
> > > > > selinux audit messages.
> > > > 
> > > > The generic quota code manages to do without either has_capability_noaudit
> > > > or ns_capable_noaudit.  I think this might be hidden behind
> > > > inode_owner_or_capable calls.  Any idea why we're different?
> > > 
> > > Actually no. Generic quota code has equivalent checks in ignore_hardlimit()
> > > function which does capable(CAP_SYS_RESOURCE) check. I guess the reason why
> > > nobody complained about generic quota code is that we call
> > > ignore_hardlimit() only if we are above hardlimit whereas XFS calls this
> > > for every transaction...
> > 
> > I guess we should aim for the same to avoid the spurious audit logs.
> > 
> > I.e. xfs_trans_alloc_ichange is currently always called either with
> > force = true or force = this capable check.  So as a first step we can
> > move the check into xfs_trans_alloc_ichange for the !force case, and the
> > propagate that through XFS_QMOPT_FORCE_RES into xfs_trans_dqresv, i.e.
> > only set XFS_QMOPT_FORCE_RES for the real forced case and instead
> > have the capable check down in xfs_trans_dqresv.
> > 
> 
> Sounds fair, I'll give it a try tomorrow.

So yeah, I agree we should change that to:

	ns_capable_noaudit(&init_user_ns, CAP_FOWNER)

Though it's also weird that XFS gates it on CAP_FOWNER whereas the VFS
checks CAP_SYS_RESOURCE.  Though I would have added this:

static inline bool
current_may_ignore_quota_limits(void)
{
	/*
	 * 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);
}

and then changed the callsites in xfs_ioctl/xfs_iops.c to:

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

The has_capability_noaudit call in xfs_fsmap.c should change to
ns_capable_noaudit.

--D