[PATCH v2 02/11] quota: reorder capability check last

[PATCH v2 02/11] quota: reorder capability check last

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

capable() calls refer to enabled LSMs whether to permit or deny the
request.  This is relevant in connection with SELinux, where a
capability check results in a policy decision and by default a denial
message on insufficient permission is issued.
It can lead to three undesired cases:
  1. A denial message is generated, even in case the operation was an
     unprivileged one and thus the syscall succeeded, creating noise.
  2. To avoid the noise from 1. the policy writer adds a rule to ignore
     those denial messages, hiding future syscalls, where the task
     performs an actual privileged operation, leading to hidden limited
     functionality of that task.
  3. To avoid the noise from 1. the policy writer adds a rule to permit
     the task the requested capability, while it does not need it,
     violating the principle of least privilege.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
---
 fs/quota/dquot.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 825c5c2e0962..5c56babf581c 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -1283,9 +1283,9 @@ 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) &&
-	       (info->dqi_format->qf_fmt_id != QFMT_VFS_OLD ||
-		!(info->dqi_flags & DQF_ROOT_SQUASH));
+	return (info->dqi_format->qf_fmt_id != QFMT_VFS_OLD ||
+		!(info->dqi_flags & DQF_ROOT_SQUASH)) &&
+		capable(CAP_SYS_RESOURCE);
 }
 
 static int dquot_add_inodes(struct dquot *dquot, qsize_t inodes,
-- 
2.47.2

[PATCH v2 03/11] ext4: reorder capability check last

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

capable() calls refer to enabled LSMs whether to permit or deny the
request.  This is relevant in connection with SELinux, where a
capability check results in a policy decision and by default a denial
message on insufficient permission is issued.
It can lead to three undesired cases:
  1. A denial message is generated, even in case the operation was an
     unprivileged one and thus the syscall succeeded, creating noise.
  2. To avoid the noise from 1. the policy writer adds a rule to ignore
     those denial messages, hiding future syscalls, where the task
     performs an actual privileged operation, leading to hidden limited
     functionality of that task.
  3. To avoid the noise from 1. the policy writer adds a rule to permit
     the task the requested capability, while it does not need it,
     violating the principle of least privilege.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
---
 fs/ext4/balloc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
index 8042ad873808..c48fd36b2d74 100644
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -649,8 +649,8 @@ static int ext4_has_free_clusters(struct ext4_sb_info *sbi,
 	/* Hm, nope.  Are (enough) root reserved clusters available? */
 	if (uid_eq(sbi->s_resuid, current_fsuid()) ||
 	    (!gid_eq(sbi->s_resgid, GLOBAL_ROOT_GID) && in_group_p(sbi->s_resgid)) ||
-	    capable(CAP_SYS_RESOURCE) ||
-	    (flags & EXT4_MB_USE_ROOT_BLOCKS)) {
+	    (flags & EXT4_MB_USE_ROOT_BLOCKS) ||
+	    capable(CAP_SYS_RESOURCE)) {
 
 		if (free_clusters >= (nclusters + dirty_clusters +
 				      resv_clusters))
-- 
2.47.2

[PATCH v2 04/11] hugetlbfs: reorder capability check last

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

capable() calls refer to enabled LSMs whether to permit or deny the
request.  This is relevant in connection with SELinux, where a
capability check results in a policy decision and by default a denial
message on insufficient permission is issued.
It can lead to three undesired cases:
  1. A denial message is generated, even in case the operation was an
     unprivileged one and thus the syscall succeeded, creating noise.
  2. To avoid the noise from 1. the policy writer adds a rule to ignore
     those denial messages, hiding future syscalls, where the task
     performs an actual privileged operation, leading to hidden limited
     functionality of that task.
  3. To avoid the noise from 1. the policy writer adds a rule to permit
     the task the requested capability, while it does not need it,
     violating the principle of least privilege.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
---
 fs/hugetlbfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 0fc179a59830..e36b0e6df720 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -1503,7 +1503,7 @@ static int can_do_hugetlb_shm(void)
 {
 	kgid_t shm_group;
 	shm_group = make_kgid(&init_user_ns, sysctl_hugetlb_shm_group);
-	return capable(CAP_IPC_LOCK) || in_group_p(shm_group);
+	return in_group_p(shm_group) || capable(CAP_IPC_LOCK);
 }
 
 static int get_hstate_idx(int page_size_log)
-- 
2.47.2

[PATCH v2 05/11] genwqe: reorder capability check last

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

capable() calls refer to enabled LSMs whether to permit or deny the
request.  This is relevant in connection with SELinux, where a
capability check results in a policy decision and by default a denial
message on insufficient permission is issued.
It can lead to three undesired cases:
  1. A denial message is generated, even in case the operation was an
     unprivileged one and thus the syscall succeeded, creating noise.
  2. To avoid the noise from 1. the policy writer adds a rule to ignore
     those denial messages, hiding future syscalls, where the task
     performs an actual privileged operation, leading to hidden limited
     functionality of that task.
  3. To avoid the noise from 1. the policy writer adds a rule to permit
     the task the requested capability, while it does not need it,
     violating the principle of least privilege.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
---
 drivers/misc/genwqe/card_dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/genwqe/card_dev.c b/drivers/misc/genwqe/card_dev.c
index 4441aca2280a..77b2d191d21c 100644
--- a/drivers/misc/genwqe/card_dev.c
+++ b/drivers/misc/genwqe/card_dev.c
@@ -461,7 +461,7 @@ static int genwqe_mmap(struct file *filp, struct vm_area_struct *vma)
 		goto free_dma_map;
 	}
 
-	if (capable(CAP_SYS_ADMIN) && (vsize > sizeof(dma_addr_t)))
+	if ((vsize > sizeof(dma_addr_t)) && capable(CAP_SYS_ADMIN))
 		*(dma_addr_t *)dma_map->k_vaddr = dma_map->dma_addr;
 
 	pfn = virt_to_phys(dma_map->k_vaddr) >> PAGE_SHIFT;
-- 
2.47.2

[PATCH v2 06/11] ubifs: reorder capability check last

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

capable() calls refer to enabled LSMs whether to permit or deny the
request.  This is relevant in connection with SELinux, where a
capability check results in a policy decision and by default a denial
message on insufficient permission is issued.
It can lead to three undesired cases:
  1. A denial message is generated, even in case the operation was an
     unprivileged one and thus the syscall succeeded, creating noise.
  2. To avoid the noise from 1. the policy writer adds a rule to ignore
     those denial messages, hiding future syscalls, where the task
     performs an actual privileged operation, leading to hidden limited
     functionality of that task.
  3. To avoid the noise from 1. the policy writer adds a rule to permit
     the task the requested capability, while it does not need it,
     violating the principle of least privilege.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: Richard Weinberger <richard@nod.at>
---
v2: split into two patches for each subsystem
---
 fs/ubifs/budget.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/ubifs/budget.c b/fs/ubifs/budget.c
index d76eb7b39f56..6137aeadec3f 100644
--- a/fs/ubifs/budget.c
+++ b/fs/ubifs/budget.c
@@ -256,8 +256,9 @@ long long ubifs_calc_available(const struct ubifs_info *c, int min_idx_lebs)
  */
 static int can_use_rp(struct ubifs_info *c)
 {
-	if (uid_eq(current_fsuid(), c->rp_uid) || capable(CAP_SYS_RESOURCE) ||
-	    (!gid_eq(c->rp_gid, GLOBAL_ROOT_GID) && in_group_p(c->rp_gid)))
+	if (uid_eq(current_fsuid(), c->rp_uid) ||
+	    (!gid_eq(c->rp_gid, GLOBAL_ROOT_GID) && in_group_p(c->rp_gid)) ||
+	    capable(CAP_SYS_RESOURCE))
 		return 1;
 	return 0;
 }
-- 
2.47.2

[PATCH v2 07/11] drm/panthor: reorder capability check last

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

capable() calls refer to enabled LSMs whether to permit or deny the
request.  This is relevant in connection with SELinux, where a
capability check results in a policy decision and by default a denial
message on insufficient permission is issued.
It can lead to three undesired cases:
  1. A denial message is generated, even in case the operation was an
     unprivileged one and thus the syscall succeeded, creating noise.
  2. To avoid the noise from 1. the policy writer adds a rule to ignore
     those denial messages, hiding future syscalls, where the task
     performs an actual privileged operation, leading to hidden limited
     functionality of that task.
  3. To avoid the noise from 1. the policy writer adds a rule to permit
     the task the requested capability, while it does not need it,
     violating the principle of least privilege.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
---
v2: split from previous patch with unrelated subsystem
---
 drivers/gpu/drm/panthor/panthor_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
index 08136e790ca0..76a10121f8a8 100644
--- a/drivers/gpu/drm/panthor/panthor_drv.c
+++ b/drivers/gpu/drm/panthor/panthor_drv.c
@@ -791,7 +791,7 @@ static int group_priority_permit(struct drm_file *file,
 		return 0;
 
 	/* Higher priorities require CAP_SYS_NICE or DRM_MASTER */
-	if (capable(CAP_SYS_NICE) || drm_is_current_master(file))
+	if (drm_is_current_master(file) || capable(CAP_SYS_NICE))
 		return 0;
 
 	return -EACCES;
-- 
2.47.2

[PATCH v2 08/11] ipv4: reorder capability check last

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

capable() calls refer to enabled LSMs whether to permit or deny the
request.  This is relevant in connection with SELinux, where a
capability check results in a policy decision and by default a denial
message on insufficient permission is issued.
It can lead to three undesired cases:
  1. A denial message is generated, even in case the operation was an
     unprivileged one and thus the syscall succeeded, creating noise.
  2. To avoid the noise from 1. the policy writer adds a rule to ignore
     those denial messages, hiding future syscalls, where the task
     performs an actual privileged operation, leading to hidden limited
     functionality of that task.
  3. To avoid the noise from 1. the policy writer adds a rule to permit
     the task the requested capability, while it does not need it,
     violating the principle of least privilege.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
---
 net/ipv4/tcp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 57df7c1d2faa..9828bc5712b7 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -3419,8 +3419,8 @@ EXPORT_SYMBOL(tcp_disconnect);
 
 static inline bool tcp_can_repair_sock(const struct sock *sk)
 {
-	return sockopt_ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN) &&
-		(sk->sk_state != TCP_LISTEN);
+	return (sk->sk_state != TCP_LISTEN) &&
+	       sockopt_ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN);
 }
 
 static int tcp_repair_set_window(struct tcp_sock *tp, sockptr_t optbuf, int len)
-- 
2.47.2

[PATCH v2 09/11] fs: reorder capability check last

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

capable() calls refer to enabled LSMs whether to permit or deny the
request.  This is relevant in connection with SELinux, where a
capability check results in a policy decision and by default a denial
message on insufficient permission is issued.
It can lead to three undesired cases:
  1. A denial message is generated, even in case the operation was an
     unprivileged one and thus the syscall succeeded, creating noise.
  2. To avoid the noise from 1. the policy writer adds a rule to ignore
     those denial messages, hiding future syscalls, where the task
     performs an actual privileged operation, leading to hidden limited
     functionality of that task.
  3. To avoid the noise from 1. the policy writer adds a rule to permit
     the task the requested capability, while it does not need it,
     violating the principle of least privilege.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Reviewed-by: Christian Brauner <brauner@kernel.org>
---
 fs/fhandle.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/fhandle.c b/fs/fhandle.c
index 3e092ae6d142..5b77b38f0510 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -303,9 +303,9 @@ static inline int may_decode_fh(struct handle_to_path_ctx *ctx,
 	if (ns_capable(root->mnt->mnt_sb->s_user_ns, CAP_SYS_ADMIN))
 		ctx->flags = HANDLE_CHECK_PERMS;
 	else if (is_mounted(root->mnt) &&
+		 !has_locked_children(real_mount(root->mnt), root->dentry) &&
 		 ns_capable(real_mount(root->mnt)->mnt_ns->user_ns,
-			    CAP_SYS_ADMIN) &&
-		 !has_locked_children(real_mount(root->mnt), root->dentry))
+			    CAP_SYS_ADMIN))
 		ctx->flags = HANDLE_CHECK_PERMS | HANDLE_CHECK_SUBTREE;
 	else
 		return -EPERM;
-- 
2.47.2

[PATCH v2 10/11] skbuff: reorder capability check last

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

capable() calls refer to enabled LSMs whether to permit or deny the
request.  This is relevant in connection with SELinux, where a
capability check results in a policy decision and by default a denial
message on insufficient permission is issued.
It can lead to three undesired cases:
  1. A denial message is generated, even in case the operation was an
     unprivileged one and thus the syscall succeeded, creating noise.
  2. To avoid the noise from 1. the policy writer adds a rule to ignore
     those denial messages, hiding future syscalls, where the task
     performs an actual privileged operation, leading to hidden limited
     functionality of that task.
  3. To avoid the noise from 1. the policy writer adds a rule to permit
     the task the requested capability, while it does not need it,
     violating the principle of least privilege.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
---
 net/core/skbuff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index b1c81687e9d8..7ed538e15b56 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1566,7 +1566,7 @@ int mm_account_pinned_pages(struct mmpin *mmp, size_t size)
 	unsigned long max_pg, num_pg, new_pg, old_pg, rlim;
 	struct user_struct *user;
 
-	if (capable(CAP_IPC_LOCK) || !size)
+	if (!size || capable(CAP_IPC_LOCK))
 		return 0;
 
 	rlim = rlimit(RLIMIT_MEMLOCK);
-- 
2.47.2

[PATCH v2 11/11] infiniband: reorder capability check last

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

capable() calls refer to enabled LSMs whether to permit or deny the
request.  This is relevant in connection with SELinux, where a
capability check results in a policy decision and by default a denial
message on insufficient permission is issued.
It can lead to three undesired cases:
  1. A denial message is generated, even in case the operation was an
     unprivileged one and thus the syscall succeeded, creating noise.
  2. To avoid the noise from 1. the policy writer adds a rule to ignore
     those denial messages, hiding future syscalls, where the task
     performs an actual privileged operation, leading to hidden limited
     functionality of that task.
  3. To avoid the noise from 1. the policy writer adds a rule to permit
     the task the requested capability, while it does not need it,
     violating the principle of least privilege.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
---
 drivers/infiniband/hw/mlx5/devx.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/devx.c b/drivers/infiniband/hw/mlx5/devx.c
index 4186884c66e1..39304cae5b10 100644
--- a/drivers/infiniband/hw/mlx5/devx.c
+++ b/drivers/infiniband/hw/mlx5/devx.c
@@ -136,12 +136,14 @@ int mlx5_ib_devx_create(struct mlx5_ib_dev *dev, bool is_user)
 		return -EINVAL;
 
 	uctx = MLX5_ADDR_OF(create_uctx_in, in, uctx);
-	if (is_user && capable(CAP_NET_RAW) &&
-	    (MLX5_CAP_GEN(dev->mdev, uctx_cap) & MLX5_UCTX_CAP_RAW_TX))
+	if (is_user &&
+	    (MLX5_CAP_GEN(dev->mdev, uctx_cap) & MLX5_UCTX_CAP_RAW_TX) &&
+	    capable(CAP_NET_RAW))
 		cap |= MLX5_UCTX_CAP_RAW_TX;
-	if (is_user && capable(CAP_SYS_RAWIO) &&
+	if (is_user &&
 	    (MLX5_CAP_GEN(dev->mdev, uctx_cap) &
-	     MLX5_UCTX_CAP_INTERNAL_DEV_RES))
+	     MLX5_UCTX_CAP_INTERNAL_DEV_RES) &&
+	    capable(CAP_SYS_RAWIO))
 		cap |= MLX5_UCTX_CAP_INTERNAL_DEV_RES;
 
 	MLX5_SET(create_uctx_in, in, opcode, MLX5_CMD_OP_CREATE_UCTX);
-- 
2.47.2

[PATCH v2 01/11] coccinelle: Add script to reorder capable() calls

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

capable() calls refer to enabled LSMs whether to permit or deny the
request.  This is relevant in connection with SELinux, where a
capability check results in a policy decision and by default a denial
message on insufficient permission is issued.
It can lead to three undesired cases:
  1. A denial message is generated, even in case the operation was an
     unprivileged one and thus the syscall succeeded, creating noise.
  2. To avoid the noise from 1. the policy writer adds a rule to ignore
     those denial messages, hiding future syscalls, where the task
     performs an actual privileged operation, leading to hidden limited
     functionality of that task.
  3. To avoid the noise from 1. the policy writer adds a rule to permit
     the task the requested capability, while it does not need it,
     violating the principle of least privilege.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
---
 MAINTAINERS                                |  1 +
 scripts/coccinelle/api/capable_order.cocci | 98 ++++++++++++++++++++++
 2 files changed, 99 insertions(+)
 create mode 100644 scripts/coccinelle/api/capable_order.cocci

diff --git a/MAINTAINERS b/MAINTAINERS
index 8e0736dc2ee0..b1d1c801765b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5196,6 +5196,7 @@ F:	include/linux/capability.h
 F:	include/trace/events/capability.h
 F:	include/uapi/linux/capability.h
 F:	kernel/capability.c
+F:	scripts/coccinelle/api/capable_order.cocci
 F:	security/commoncap.c
 
 CAPELLA MICROSYSTEMS LIGHT SENSOR DRIVER
diff --git a/scripts/coccinelle/api/capable_order.cocci b/scripts/coccinelle/api/capable_order.cocci
new file mode 100644
index 000000000000..4150d91b0f33
--- /dev/null
+++ b/scripts/coccinelle/api/capable_order.cocci
@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Checks for capable() calls of the left side of a binary expression.
+/// Reordering might avoid needless checks, LSM log messages, and more
+/// restrictive LSM security policies (e.g. SELinux).
+/// Can report false positives if the righthand side contains a nested
+/// capability check or has side effects.
+///
+// Confidence: Moderate
+// Copyright: (C) 2024 Christian Göttsche.
+// Options: --no-includes --include-headers
+// Keywords: capable, ns_capable, sockopt_ns_capable
+//
+
+virtual patch
+virtual context
+virtual org
+virtual report
+
+//----------------------------------------------------------
+//  Pattern to ignore
+//----------------------------------------------------------
+
+@ignore@
+identifier F1 = { capable, ns_capable, sockopt_ns_capable };
+identifier F2 = { capable, ns_capable, sockopt_ns_capable };
+binary operator op,op1,op2;
+expression E;
+position p;
+@@
+
+(
+F1@p(...) op F2(...)
+|
+E op1 F1@p(...) op2 F2(...)
+)
+
+
+//----------------------------------------------------------
+//  For patch mode
+//----------------------------------------------------------
+
+@ depends on patch@
+identifier F = { capable, ns_capable, sockopt_ns_capable };
+binary operator op,op1,op2;
+expression E,E1,E2;
+expression list EL;
+position p != ignore.p;
+@@
+
+(
+-  F@p(EL) op E
++  E op F(EL)
+|
+-  E1 op1 F@p(EL) op2 E2
++  E1 op1 E2 op2 F(EL)
+)
+
+
+//----------------------------------------------------------
+//  For context mode
+//----------------------------------------------------------
+
+@r1 depends on !patch exists@
+identifier F = { capable, ns_capable, sockopt_ns_capable };
+binary operator op,op1,op2;
+expression E, E1, E2;
+position p != ignore.p;
+@@
+
+(
+*  F@p(...) op E
+|
+*  E1 op1 F@p(...) op2 E2
+)
+
+
+//----------------------------------------------------------
+//  For org mode
+//----------------------------------------------------------
+
+@script:python depends on org@
+p << r1.p;
+@@
+
+cocci.print_main("WARNING opportunity for capable reordering",p)
+
+
+//----------------------------------------------------------
+//  For report mode
+//----------------------------------------------------------
+
+@script:python depends on report@
+p << r1.p;
+@@
+
+msg = "WARNING opportunity for capable reordering"
+coccilib.report.print_report(p[0], msg)
-- 
2.47.2

Re: [PATCH v2 01/11] coccinelle: Add script to reorder capable() calls

[Casey Schaufler]

On 3/2/2025 8:06 AM, Christian Göttsche wrote:
> From: Christian Göttsche <cgzones@googlemail.com>
>
> capable() calls refer to enabled LSMs whether to permit or deny the
> request.  This is relevant in connection with SELinux, where a
> capability check results in a policy decision and by default a denial
> message on insufficient permission is issued.
> It can lead to three undesired cases:
>   1. A denial message is generated, even in case the operation was an
>      unprivileged one and thus the syscall succeeded, creating noise.
>   2. To avoid the noise from 1. the policy writer adds a rule to ignore
>      those denial messages, hiding future syscalls, where the task
>      performs an actual privileged operation, leading to hidden limited
>      functionality of that task.
>   3. To avoid the noise from 1. the policy writer adds a rule to permit
>      the task the requested capability, while it does not need it,
>      violating the principle of least privilege.

What steps are you taking to ensure that these changes do not
negatively impact LSMs other than SELinux? At a glance, I don't
see that there is likely to be a problem. I do see a possibility
that changes in error returns could break test suites and, more
importantly, applications that are careful about using privileged
operations.

>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> Reviewed-by: Serge Hallyn <serge@hallyn.com>
> ---
>  MAINTAINERS                                |  1 +
>  scripts/coccinelle/api/capable_order.cocci | 98 ++++++++++++++++++++++
>  2 files changed, 99 insertions(+)
>  create mode 100644 scripts/coccinelle/api/capable_order.cocci
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8e0736dc2ee0..b1d1c801765b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5196,6 +5196,7 @@ F:	include/linux/capability.h
>  F:	include/trace/events/capability.h
>  F:	include/uapi/linux/capability.h
>  F:	kernel/capability.c
> +F:	scripts/coccinelle/api/capable_order.cocci
>  F:	security/commoncap.c
>  
>  CAPELLA MICROSYSTEMS LIGHT SENSOR DRIVER
> diff --git a/scripts/coccinelle/api/capable_order.cocci b/scripts/coccinelle/api/capable_order.cocci
> new file mode 100644
> index 000000000000..4150d91b0f33
> --- /dev/null
> +++ b/scripts/coccinelle/api/capable_order.cocci
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +///
> +/// Checks for capable() calls of the left side of a binary expression.
> +/// Reordering might avoid needless checks, LSM log messages, and more
> +/// restrictive LSM security policies (e.g. SELinux).
> +/// Can report false positives if the righthand side contains a nested
> +/// capability check or has side effects.
> +///
> +// Confidence: Moderate
> +// Copyright: (C) 2024 Christian Göttsche.
> +// Options: --no-includes --include-headers
> +// Keywords: capable, ns_capable, sockopt_ns_capable
> +//
> +
> +virtual patch
> +virtual context
> +virtual org
> +virtual report
> +
> +//----------------------------------------------------------
> +//  Pattern to ignore
> +//----------------------------------------------------------
> +
> +@ignore@
> +identifier F1 = { capable, ns_capable, sockopt_ns_capable };
> +identifier F2 = { capable, ns_capable, sockopt_ns_capable };
> +binary operator op,op1,op2;
> +expression E;
> +position p;
> +@@
> +
> +(
> +F1@p(...) op F2(...)
> +|
> +E op1 F1@p(...) op2 F2(...)
> +)
> +
> +
> +//----------------------------------------------------------
> +//  For patch mode
> +//----------------------------------------------------------
> +
> +@ depends on patch@
> +identifier F = { capable, ns_capable, sockopt_ns_capable };
> +binary operator op,op1,op2;
> +expression E,E1,E2;
> +expression list EL;
> +position p != ignore.p;
> +@@
> +
> +(
> +-  F@p(EL) op E
> ++  E op F(EL)
> +|
> +-  E1 op1 F@p(EL) op2 E2
> ++  E1 op1 E2 op2 F(EL)
> +)
> +
> +
> +//----------------------------------------------------------
> +//  For context mode
> +//----------------------------------------------------------
> +
> +@r1 depends on !patch exists@
> +identifier F = { capable, ns_capable, sockopt_ns_capable };
> +binary operator op,op1,op2;
> +expression E, E1, E2;
> +position p != ignore.p;
> +@@
> +
> +(
> +*  F@p(...) op E
> +|
> +*  E1 op1 F@p(...) op2 E2
> +)
> +
> +
> +//----------------------------------------------------------
> +//  For org mode
> +//----------------------------------------------------------
> +
> +@script:python depends on org@
> +p << r1.p;
> +@@
> +
> +cocci.print_main("WARNING opportunity for capable reordering",p)
> +
> +
> +//----------------------------------------------------------
> +//  For report mode
> +//----------------------------------------------------------
> +
> +@script:python depends on report@
> +p << r1.p;
> +@@
> +
> +msg = "WARNING opportunity for capable reordering"
> +coccilib.report.print_report(p[0], msg)

Re: [PATCH v2 08/11] ipv4: reorder capability check last

[Eric Dumazet]

On Sun, Mar 2, 2025 at 5:07 PM Christian Göttsche
<cgoettsche@seltendoof.de> wrote:
>
> From: Christian Göttsche <cgzones@googlemail.com>
>
> capable() calls refer to enabled LSMs whether to permit or deny the
> request.  This is relevant in connection with SELinux, where a
> capability check results in a policy decision and by default a denial
> message on insufficient permission is issued.
> It can lead to three undesired cases:
>   1. A denial message is generated, even in case the operation was an
>      unprivileged one and thus the syscall succeeded, creating noise.
>   2. To avoid the noise from 1. the policy writer adds a rule to ignore
>      those denial messages, hiding future syscalls, where the task
>      performs an actual privileged operation, leading to hidden limited
>      functionality of that task.
>   3. To avoid the noise from 1. the policy writer adds a rule to permit
>      the task the requested capability, while it does not need it,
>      violating the principle of least privilege.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> Reviewed-by: Serge Hallyn <serge@hallyn.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH v2 01/11] coccinelle: Add script to reorder capable() calls

[Christian Göttsche]

On Sun, 2 Mar 2025 at 17:53, Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 3/2/2025 8:06 AM, Christian Göttsche wrote:
> > From: Christian Göttsche <cgzones@googlemail.com>
> >
> > capable() calls refer to enabled LSMs whether to permit or deny the
> > request.  This is relevant in connection with SELinux, where a
> > capability check results in a policy decision and by default a denial
> > message on insufficient permission is issued.
> > It can lead to three undesired cases:
> >   1. A denial message is generated, even in case the operation was an
> >      unprivileged one and thus the syscall succeeded, creating noise.
> >   2. To avoid the noise from 1. the policy writer adds a rule to ignore
> >      those denial messages, hiding future syscalls, where the task
> >      performs an actual privileged operation, leading to hidden limited
> >      functionality of that task.
> >   3. To avoid the noise from 1. the policy writer adds a rule to permit
> >      the task the requested capability, while it does not need it,
> >      violating the principle of least privilege.
>
> What steps are you taking to ensure that these changes do not
> negatively impact LSMs other than SELinux? At a glance, I don't
> see that there is likely to be a problem. I do see a possibility
> that changes in error returns could break test suites and, more
> importantly, applications that are careful about using privileged
> operations.

Checks are only reordered where the current right-hand side is
side-effect free, e.g. a comparison.
Whether a branch is taken or not (and thus possible return values)
should not be affected.

Here is the current output of the script with the false-positives not converted:


### begin ###
diff -u -p a/kernel/capability.c b/kernel/capability.c
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -491,8 +491,8 @@ bool capable_wrt_inode_uidgid(struct mnt
{
       struct user_namespace *ns = current_user_ns();

-       return ns_capable(ns, cap) &&
-              privileged_wrt_inode_uidgid(ns, idmap, inode);
+       return privileged_wrt_inode_uidgid(ns, idmap, inode) && ns_capable(ns,
+                                              cap);
}
EXPORT_SYMBOL(capable_wrt_inode_uidgid);

diff -u -p a/kernel/pid.c b/kernel/pid.c
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -662,8 +662,7 @@ static int pid_table_root_permissions(st
               container_of(head->set, struct pid_namespace, set);
       int mode = table->mode;

-       if (ns_capable(pidns->user_ns, CAP_SYS_ADMIN) ||
-           uid_eq(current_euid(), make_kuid(pidns->user_ns, 0)))
+       if (uid_eq(current_euid(), make_kuid(pidns->user_ns, 0)) ||
ns_capable(pidns->user_ns, CAP_SYS_ADMIN))
               mode = (mode & S_IRWXU) >> 6;
       else if (in_egroup_p(make_kgid(pidns->user_ns, 0)))
               mode = (mode & S_IRWXG) >> 3;
diff -u -p a/kernel/bpf/token.c b/kernel/bpf/token.c
--- a/kernel/bpf/token.c
+++ b/kernel/bpf/token.c
@@ -10,7 +10,8 @@

static bool bpf_ns_capable(struct user_namespace *ns, int cap)
{
-       return ns_capable(ns, cap) || (cap != CAP_SYS_ADMIN &&
ns_capable(ns, CAP_SYS_ADMIN));
+       return (cap != CAP_SYS_ADMIN && ns_capable(ns, CAP_SYS_ADMIN))
|| ns_capable(ns,
+
              cap);
}

bool bpf_token_capable(const struct bpf_token *token, int cap)
diff -u -p a/debian/linux-headers-6.9.0-rc2+/usr/src/linux-headers-6.9.0-rc2+/include/linux/bpf.h
b/debian/linux-headers-6.9.0-rc2+/usr/src/linux-headers-6.9.0-rc2+/include/linux/bpf.h
--- a/debian/linux-headers-6.9.0-rc2+/usr/src/linux-headers-6.9.0-rc2+/include/linux/bpf.h
+++ b/debian/linux-headers-6.9.0-rc2+/usr/src/linux-headers-6.9.0-rc2+/include/linux/bpf.h
@@ -2715,7 +2715,7 @@ static inline int bpf_obj_get_user(const

static inline bool bpf_token_capable(const struct bpf_token *token, int cap)
{
-       return capable(cap) || (cap != CAP_SYS_ADMIN && capable(CAP_SYS_ADMIN));
+       return (cap != CAP_SYS_ADMIN && capable(CAP_SYS_ADMIN)) || capable(cap);
}

static inline void bpf_token_inc(struct bpf_token *token)
diff -u -p a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1285,8 +1285,7 @@ static ssize_t scrub_show(struct device
               && !test_bit(ARS_CANCEL, &acpi_desc->scrub_flags);
       rc = sysfs_emit(buf, "%d%s", acpi_desc->scrub_count, busy ?
"+\n" : "\n");
       /* Allow an admin to poll the busy state at a higher rate */
-       if (busy && capable(CAP_SYS_RAWIO) && !test_and_set_bit(ARS_POLL,
-                               &acpi_desc->scrub_flags)) {
+       if (busy && !test_and_set_bit(ARS_POLL,
&acpi_desc->scrub_flags) && capable(CAP_SYS_RAWIO)) {
               acpi_desc->scrub_tmo = 1;
               mod_delayed_work(nfit_wq, &acpi_desc->dwork, HZ);
       }
diff -u -p a/include/linux/bpf.h b/include/linux/bpf.h
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2830,7 +2830,7 @@ static inline int bpf_obj_get_user(const

static inline bool bpf_token_capable(const struct bpf_token *token, int cap)
{
-       return capable(cap) || (cap != CAP_SYS_ADMIN && capable(CAP_SYS_ADMIN));
+       return (cap != CAP_SYS_ADMIN && capable(CAP_SYS_ADMIN)) || capable(cap);
}

static inline void bpf_token_inc(struct bpf_token *token)
diff -u -p a/kernel/user_namespace.c b/kernel/user_namespace.c
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -1194,8 +1194,7 @@ static bool new_idmap_permitted(const st
        * (CAP_SETUID or CAP_SETGID) over the parent user namespace.
        * And the opener of the id file also has the appropriate capability.
        */
-       if (ns_capable(ns->parent, cap_setid) &&
-           file_ns_capable(file, ns->parent, cap_setid))
+       if (file_ns_capable(file, ns->parent, cap_setid) &&
ns_capable(ns->parent, cap_setid))
               return true;

       return false;
### end ###

>
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> > Reviewed-by: Serge Hallyn <serge@hallyn.com>
> > ---
> >  MAINTAINERS                                |  1 +
> >  scripts/coccinelle/api/capable_order.cocci | 98 ++++++++++++++++++++++
> >  2 files changed, 99 insertions(+)
> >  create mode 100644 scripts/coccinelle/api/capable_order.cocci
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 8e0736dc2ee0..b1d1c801765b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -5196,6 +5196,7 @@ F:      include/linux/capability.h
> >  F:   include/trace/events/capability.h
> >  F:   include/uapi/linux/capability.h
> >  F:   kernel/capability.c
> > +F:   scripts/coccinelle/api/capable_order.cocci
> >  F:   security/commoncap.c
> >
> >  CAPELLA MICROSYSTEMS LIGHT SENSOR DRIVER
> > diff --git a/scripts/coccinelle/api/capable_order.cocci b/scripts/coccinelle/api/capable_order.cocci
> > new file mode 100644
> > index 000000000000..4150d91b0f33
> > --- /dev/null
> > +++ b/scripts/coccinelle/api/capable_order.cocci
> > @@ -0,0 +1,98 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +///
> > +/// Checks for capable() calls of the left side of a binary expression.
> > +/// Reordering might avoid needless checks, LSM log messages, and more
> > +/// restrictive LSM security policies (e.g. SELinux).
> > +/// Can report false positives if the righthand side contains a nested
> > +/// capability check or has side effects.
> > +///
> > +// Confidence: Moderate
> > +// Copyright: (C) 2024 Christian Göttsche.
> > +// Options: --no-includes --include-headers
> > +// Keywords: capable, ns_capable, sockopt_ns_capable
> > +//
> > +
> > +virtual patch
> > +virtual context
> > +virtual org
> > +virtual report
> > +
> > +//----------------------------------------------------------
> > +//  Pattern to ignore
> > +//----------------------------------------------------------
> > +
> > +@ignore@
> > +identifier F1 = { capable, ns_capable, sockopt_ns_capable };
> > +identifier F2 = { capable, ns_capable, sockopt_ns_capable };
> > +binary operator op,op1,op2;
> > +expression E;
> > +position p;
> > +@@
> > +
> > +(
> > +F1@p(...) op F2(...)
> > +|
> > +E op1 F1@p(...) op2 F2(...)
> > +)
> > +
> > +
> > +//----------------------------------------------------------
> > +//  For patch mode
> > +//----------------------------------------------------------
> > +
> > +@ depends on patch@
> > +identifier F = { capable, ns_capable, sockopt_ns_capable };
> > +binary operator op,op1,op2;
> > +expression E,E1,E2;
> > +expression list EL;
> > +position p != ignore.p;
> > +@@
> > +
> > +(
> > +-  F@p(EL) op E
> > ++  E op F(EL)
> > +|
> > +-  E1 op1 F@p(EL) op2 E2
> > ++  E1 op1 E2 op2 F(EL)
> > +)
> > +
> > +
> > +//----------------------------------------------------------
> > +//  For context mode
> > +//----------------------------------------------------------
> > +
> > +@r1 depends on !patch exists@
> > +identifier F = { capable, ns_capable, sockopt_ns_capable };
> > +binary operator op,op1,op2;
> > +expression E, E1, E2;
> > +position p != ignore.p;
> > +@@
> > +
> > +(
> > +*  F@p(...) op E
> > +|
> > +*  E1 op1 F@p(...) op2 E2
> > +)
> > +
> > +
> > +//----------------------------------------------------------
> > +//  For org mode
> > +//----------------------------------------------------------
> > +
> > +@script:python depends on org@
> > +p << r1.p;
> > +@@
> > +
> > +cocci.print_main("WARNING opportunity for capable reordering",p)
> > +
> > +
> > +//----------------------------------------------------------
> > +//  For report mode
> > +//----------------------------------------------------------
> > +
> > +@script:python depends on report@
> > +p << r1.p;
> > +@@
> > +
> > +msg = "WARNING opportunity for capable reordering"
> > +coccilib.report.print_report(p[0], msg)

Re: [PATCH v2 06/11] ubifs: reorder capability check last

[Zhihao Cheng]

在 2025/3/3 0:06, Christian Göttsche 写道:
> From: Christian Göttsche <cgzones@googlemail.com>
> 
> capable() calls refer to enabled LSMs whether to permit or deny the
> request.  This is relevant in connection with SELinux, where a
> capability check results in a policy decision and by default a denial
> message on insufficient permission is issued.
> It can lead to three undesired cases:
>    1. A denial message is generated, even in case the operation was an
>       unprivileged one and thus the syscall succeeded, creating noise.
>    2. To avoid the noise from 1. the policy writer adds a rule to ignore
>       those denial messages, hiding future syscalls, where the task
>       performs an actual privileged operation, leading to hidden limited
>       functionality of that task.
>    3. To avoid the noise from 1. the policy writer adds a rule to permit
>       the task the requested capability, while it does not need it,
>       violating the principle of least privilege.
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> Reviewed-by: Serge Hallyn <serge@hallyn.com>
> Acked-by: Richard Weinberger <richard@nod.at>
> ---
> v2: split into two patches for each subsystem
> ---
>   fs/ubifs/budget.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Zhihao Cheng <chengzhihao1@huawei.com>
> 
> diff --git a/fs/ubifs/budget.c b/fs/ubifs/budget.c
> index d76eb7b39f56..6137aeadec3f 100644
> --- a/fs/ubifs/budget.c
> +++ b/fs/ubifs/budget.c
> @@ -256,8 +256,9 @@ long long ubifs_calc_available(const struct ubifs_info *c, int min_idx_lebs)
>    */
>   static int can_use_rp(struct ubifs_info *c)
>   {
> -	if (uid_eq(current_fsuid(), c->rp_uid) || capable(CAP_SYS_RESOURCE) ||
> -	    (!gid_eq(c->rp_gid, GLOBAL_ROOT_GID) && in_group_p(c->rp_gid)))
> +	if (uid_eq(current_fsuid(), c->rp_uid) ||
> +	    (!gid_eq(c->rp_gid, GLOBAL_ROOT_GID) && in_group_p(c->rp_gid)) ||
> +	    capable(CAP_SYS_RESOURCE))
>   		return 1;
>   	return 0;
>   }
> 

Re: [PATCH v2 11/11] infiniband: reorder capability check last

[Leon Romanovsky]

On Sun, Mar 02, 2025 at 05:06:47PM +0100, Christian Göttsche wrote:
> From: Christian Göttsche <cgzones@googlemail.com>
> 
> capable() calls refer to enabled LSMs whether to permit or deny the
> request.  This is relevant in connection with SELinux, where a
> capability check results in a policy decision and by default a denial
> message on insufficient permission is issued.
> It can lead to three undesired cases:
>   1. A denial message is generated, even in case the operation was an
>      unprivileged one and thus the syscall succeeded, creating noise.
>   2. To avoid the noise from 1. the policy writer adds a rule to ignore
>      those denial messages, hiding future syscalls, where the task
>      performs an actual privileged operation, leading to hidden limited
>      functionality of that task.
>   3. To avoid the noise from 1. the policy writer adds a rule to permit
>      the task the requested capability, while it does not need it,
>      violating the principle of least privilege.
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> Reviewed-by: Serge Hallyn <serge@hallyn.com>
> ---
>  drivers/infiniband/hw/mlx5/devx.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

Thanks, applied.
https://web.git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/commit/?h=wip/leon-for-next&id=3745242ad1e1c07d5990b33764529eb13565db44

Re: [PATCH v2 03/11] ext4: reorder capability check last

[Jan Kara]

On Sun 02-03-25 17:06:39, Christian Göttsche wrote:
> From: Christian Göttsche <cgzones@googlemail.com>
> 
> capable() calls refer to enabled LSMs whether to permit or deny the
> request.  This is relevant in connection with SELinux, where a
> capability check results in a policy decision and by default a denial
> message on insufficient permission is issued.
> It can lead to three undesired cases:
>   1. A denial message is generated, even in case the operation was an
>      unprivileged one and thus the syscall succeeded, creating noise.
>   2. To avoid the noise from 1. the policy writer adds a rule to ignore
>      those denial messages, hiding future syscalls, where the task
>      performs an actual privileged operation, leading to hidden limited
>      functionality of that task.
>   3. To avoid the noise from 1. the policy writer adds a rule to permit
>      the task the requested capability, while it does not need it,
>      violating the principle of least privilege.
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> Reviewed-by: Serge Hallyn <serge@hallyn.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/balloc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
> index 8042ad873808..c48fd36b2d74 100644
> --- a/fs/ext4/balloc.c
> +++ b/fs/ext4/balloc.c
> @@ -649,8 +649,8 @@ static int ext4_has_free_clusters(struct ext4_sb_info *sbi,
>  	/* Hm, nope.  Are (enough) root reserved clusters available? */
>  	if (uid_eq(sbi->s_resuid, current_fsuid()) ||
>  	    (!gid_eq(sbi->s_resgid, GLOBAL_ROOT_GID) && in_group_p(sbi->s_resgid)) ||
> -	    capable(CAP_SYS_RESOURCE) ||
> -	    (flags & EXT4_MB_USE_ROOT_BLOCKS)) {
> +	    (flags & EXT4_MB_USE_ROOT_BLOCKS) ||
> +	    capable(CAP_SYS_RESOURCE)) {
>  
>  		if (free_clusters >= (nclusters + dirty_clusters +
>  				      resv_clusters))
> -- 
> 2.47.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 10/11] skbuff: reorder capability check last

[Willem de Bruijn]

Christian Göttsche wrote:
> From: Christian Göttsche <cgzones@googlemail.com>
> 
> capable() calls refer to enabled LSMs whether to permit or deny the
> request.  This is relevant in connection with SELinux, where a
> capability check results in a policy decision and by default a denial
> message on insufficient permission is issued.
> It can lead to three undesired cases:
>   1. A denial message is generated, even in case the operation was an
>      unprivileged one and thus the syscall succeeded, creating noise.
>   2. To avoid the noise from 1. the policy writer adds a rule to ignore
>      those denial messages, hiding future syscalls, where the task
>      performs an actual privileged operation, leading to hidden limited
>      functionality of that task.
>   3. To avoid the noise from 1. the policy writer adds a rule to permit
>      the task the requested capability, while it does not need it,
>      violating the principle of least privilege.
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> Reviewed-by: Serge Hallyn <serge@hallyn.com>

Similar to Paolo's response to patch 7: these networking patches
should probably go through net-next.

> ---
>  net/core/skbuff.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index b1c81687e9d8..7ed538e15b56 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1566,7 +1566,7 @@ int mm_account_pinned_pages(struct mmpin *mmp, size_t size)
>  	unsigned long max_pg, num_pg, new_pg, old_pg, rlim;
>  	struct user_struct *user;
>  
> -	if (capable(CAP_IPC_LOCK) || !size)
> +	if (!size || capable(CAP_IPC_LOCK))
>  		return 0;

Not sure that this case is relevant:

Unlike most other capable checks, this does not protect a privileged
operation and returns with error for unprivileged users.

It offers a shortcut to privileged users to avoid memory accounting,
but continues in the comon case that the user is not privileged.

So the common case here is to generate denial messages when LSMs are
enabled. size 0 is not likely, so swapping the order is unlikely to
significantly change the number of denial messages.

>  
>  	rlim = rlimit(RLIMIT_MEMLOCK);
> -- 
> 2.47.2
> 

Re: [PATCH v2 01/11] coccinelle: Add script to reorder capable() calls

[Theodore Ts'o]

On Sun, 02 Mar 2025 17:06:48 +0100, Christian Göttsche wrote:
> capable() calls refer to enabled LSMs whether to permit or deny the
> request.  This is relevant in connection with SELinux, where a
> capability check results in a policy decision and by default a denial
> message on insufficient permission is issued.
> It can lead to three undesired cases:
>   1. A denial message is generated, even in case the operation was an
>      unprivileged one and thus the syscall succeeded, creating noise.
>   2. To avoid the noise from 1. the policy writer adds a rule to ignore
>      those denial messages, hiding future syscalls, where the task
>      performs an actual privileged operation, leading to hidden limited
>      functionality of that task.
>   3. To avoid the noise from 1. the policy writer adds a rule to permit
>      the task the requested capability, while it does not need it,
>      violating the principle of least privilege.
> 
> [...]

Applied, thanks!

[03/11] ext4: reorder capability check last
        commit: 26f5784d44c3f824c864245b506db809b51053cf

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>