[PATCH v4 0/5] Fix quota evasion on xfs and add capable_noaudit

[PATCH v4 0/5] Fix quota evasion on xfs and add capable_noaudit

[cem]

From: Carlos Maiolino <cem@kernel.org>

Hi there.

This is the a new version of the series I've been working on
to fix a quota evasion issue on xfs. I've got to jump to more urgent
matters before the previous one so my apologies for the time it took to
cook this new version.

The changes to this version compared to the previous one are minor, only
comments and indentation related. This new version has also been rebased
on top of current xfs's for-next branch. A description of the changes
between this and the V3 version are below and more detailed on individual
patches.

This bug has originally been introduced by accident while turning off audit
messages while checking quota limits in xfs by replacing capable() calls by
as_capability_noaudit().

This series concatenates both series I sent for xfs and capabilities
infrastructure as they are dependent.

The first patch fix the xfs bug in a way that makes it easily portable
to older LTS kernels.

From second patch and beyond, it adds a new helper for the capabilities
framework named capable_noaudit() which as the same semantics as
capable() but without generating audit messages.
The following patches then replaces both generic quota call to
capable() and properly update xfs code to use this new helper.

Last but not least this unexport has_capability_noaudit which had been
previously exported.

Giving this affects different subsystems, I think it would be easier to
pull everything from a single tree (as long as everything is properly
reviewed of course).

Serge, Honza, are you guys ok if I pull those patches and send them to
Linus through xfs tree so we don't need to split the series?

Also I kept most of the RwB tags from previous versions as there were no
code changes but minor context updates to fix the conflicts.

Serge, I removed your RwB on purpose from the patch 2 as I updated the
comment, so I want to make sure you are ok with the new comment.

Changelog from the last state of these patches:

Patch2: Add more details to the capable_noaudit documentation specifying
	it checks for the process effective capabilities.

Patch4: Fix identation changes introduced in previous versions

Carlos Maiolino (5):
  xfs: fix capability check in xfs
  capability: Add new capable_noaudit
  quota: Don't issue audit messages on quota enforcing
  xfs: replace ns_capable_noaudit
  capability: unexport has_capability_noaudit

 fs/quota/dquot.c           |  2 +-
 fs/xfs/xfs_fsmap.c         |  3 +--
 fs/xfs/xfs_ioctl.c         |  2 +-
 fs/xfs/xfs_iops.c          |  2 +-
 include/linux/capability.h |  5 +++++
 kernel/capability.c        | 19 ++++++++++++++++++-
 6 files changed, 27 insertions(+), 6 deletions(-)

-- 
2.55.0

[PATCH v4 1/5] xfs: fix capability 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
Reported-by: Dr. Thomas Orgis <thomas.orgis@uni-hamburg.de>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/xfs_fsmap.c | 2 +-
 fs/xfs/xfs_ioctl.c | 2 +-
 fs/xfs/xfs_iops.c  | 2 +-
 3 files changed, 3 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 1b53701bebea..1a8af827dde1 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 6339f4956ecb..7a8c77fdcf68 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -835,7 +835,7 @@ 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.55.0

Re: [PATCH v4 1/5] xfs: fix capability check in xfs

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v4 2/5] capability: Add new capable_noaudit

[cem]

From: Carlos Maiolino <cem@kernel.org>

In some situations (quota enforcement bypass in this case) we'd like to
check for a specific capability without triggering spurious audit
messages from security modules like selinux.

Add a new helper so we don't need to use ns_capable_noaudit() directly.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---

V4: Mention capable_noaudit check for process effective capabilities
V3: remove the extern declaration

 include/linux/capability.h |  5 +++++
 kernel/capability.c        | 18 ++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/include/linux/capability.h b/include/linux/capability.h
index 37db92b3d6f8..f8532d92fcad 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -145,6 +145,7 @@ extern bool has_capability_noaudit(struct task_struct *t, int cap);
 extern bool has_ns_capability_noaudit(struct task_struct *t,
 				      struct user_namespace *ns, int cap);
 extern bool capable(int cap);
+bool capable_noaudit(int cap);
 extern bool ns_capable(struct user_namespace *ns, int cap);
 extern bool ns_capable_noaudit(struct user_namespace *ns, int cap);
 extern bool ns_capable_setid(struct user_namespace *ns, int cap);
@@ -167,6 +168,10 @@ static inline bool capable(int cap)
 {
 	return true;
 }
+static inline bool capable_noaudit(int cap)
+{
+	return true;
+}
 static inline bool ns_capable(struct user_namespace *ns, int cap)
 {
 	return true;
diff --git a/kernel/capability.c b/kernel/capability.c
index 829f49ae07b9..f4a7f1963c9d 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -416,6 +416,24 @@ bool capable(int cap)
 	return ns_capable(&init_user_ns, cap);
 }
 EXPORT_SYMBOL(capable);
+
+/**
+ * capable_noaudit - Determine if the current task has a superior
+ * capability in effect by checking the process's effective
+ * capabilities (unaudited).
+ * @cap: The capability to be tested for
+ *
+ * This is the same as capable(), except it uses CAP_OPT_NOAUDIT as to prevent
+ * issuing spurious audit messages.
+ *
+ * This sets PF_SUPERPRIV on the task if the capability is available on the
+ * assumption that it's about to be used.
+ */
+bool capable_noaudit(int cap)
+{
+	return ns_capable_noaudit(&init_user_ns, cap);
+}
+EXPORT_SYMBOL(capable_noaudit);
 #endif /* CONFIG_MULTIUSER */
 
 /**
-- 
2.55.0

[PATCH v4 3/5] quota: Don't issue audit messages on quota enforcing

[cem]

From: Carlos Maiolino <cem@kernel.org>

Calling capable() to determine if we can bypass quota enforcement or not
can trigger spurious audit messages. We don't really require it here so
just use the capable_noaudit() version.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Acked-by: Jan Kara <jack@suse.cz>
---
 fs/quota/dquot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 9850de3955d3..dab93422a57b 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -1308,7 +1308,7 @@ static int ignore_hardlimit(struct dquot *dquot)
 {
 	struct mem_dqinfo *info = &sb_dqopt(dquot->dq_sb)->info[dquot->dq_id.type];
 
-	return capable(CAP_SYS_RESOURCE) &&
+	return capable_noaudit(CAP_SYS_RESOURCE) &&
 	       (info->dqi_format->qf_fmt_id != QFMT_VFS_OLD ||
 		!(info->dqi_flags & DQF_ROOT_SQUASH));
 }
-- 
2.55.0

[PATCH v4 4/5] xfs: replace ns_capable_noaudit

[cem]

From: Carlos Maiolino <cem@kernel.org>

Now that capable_noaudit() is available, we don't need to keep
using ns_capable_noaudit() and specifying the usernaspace every single
time.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
V4: Fix indentation on xfs_ioctl_setattr_get_trans() changes

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

diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
index 7c79fbe0a74c..041bb2105ec6 100644
--- a/fs/xfs/xfs_fsmap.c
+++ b/fs/xfs/xfs_fsmap.c
@@ -1174,8 +1174,7 @@ xfs_getfsmap(
 	if (!xfs_getfsmap_check_keys(&head->fmh_keys[0], &head->fmh_keys[1]))
 		return -EINVAL;
 
-	use_rmap = xfs_has_rmapbt(mp) &&
-		   ns_capable_noaudit(&init_user_ns, CAP_SYS_ADMIN);
+	use_rmap = xfs_has_rmapbt(mp) && capable_noaudit(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 1a8af827dde1..96ca3e480cb9 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,
-			ns_capable_noaudit(&init_user_ns, CAP_FOWNER), &tp);
+			capable_noaudit(CAP_FOWNER), &tp);
 	if (error)
 		goto out_error;
 
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 7a8c77fdcf68..3f36d6e7b917 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -835,7 +835,7 @@ xfs_setattr_nonsize(
 	}
 
 	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL,
-			ns_capable_noaudit(&init_user_ns, CAP_FOWNER), &tp);
+			capable_noaudit(CAP_FOWNER), &tp);
 	if (error)
 		goto out_dqrele;
 
-- 
2.55.0

Re: [PATCH v4 4/5] xfs: replace ns_capable_noaudit

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v4 5/5] capability: unexport has_capability_noaudit

[cem]

From: Carlos Maiolino <cem@kernel.org>

This has been originally exported to be used in xfs. Giving we are not
using it anymore, unexport for consistency.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 kernel/capability.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/capability.c b/kernel/capability.c
index f4a7f1963c9d..90e6ab62f6db 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -326,7 +326,6 @@ 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,
-- 
2.55.0