linux-kernel - Re: [PATCH v11 2/2] binder: report txn errors via generic netlink

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z371VdHmZ3FVdrEI@google.com>
Date: Wed, 8 Jan 2025 21:59:49 +0000
From: Carlos Llamas <cmllamas@...gle.com>
To: Li Li <dualli@...omium.org>
Cc: dualli@...gle.com, corbet@....net, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	donald.hunter@...il.com, gregkh@...uxfoundation.org,
	arve@...roid.com, tkjos@...roid.com, maco@...roid.com,
	joel@...lfernandes.org, brauner@...nel.org, surenb@...gle.com,
	arnd@...db.de, masahiroy@...nel.org, bagasdotme@...il.com,
	horms@...nel.org, linux-kernel@...r.kernel.org,
	linux-doc@...r.kernel.org, netdev@...r.kernel.org,
	hridya@...gle.com, smoreland@...gle.com, kernel-team@...roid.com
Subject: Re: [PATCH v11 2/2] binder: report txn errors via generic netlink

On Wed, Jan 08, 2025 at 11:56:35AM -0800, Li Li wrote:
> This is a valid concern. Adding GENL_ADMIN_PERM should be enough to solve it.

Right! That seems to ask the genl stack to check for CAP_NET_ADMIN:

  if ((op.flags & GENL_ADMIN_PERM) &&
      !netlink_capable(skb, CAP_NET_ADMIN))
          return -EPERM;

... which is a much better option and we could drop the portid check to
validate permissions. Something like the following (untested)?

diff --git a/Documentation/netlink/specs/binder.yaml b/Documentation/netlink/specs/binder.yaml
index 23f26c83a7c9..a0ef31cba666 100644
--- a/Documentation/netlink/specs/binder.yaml
+++ b/Documentation/netlink/specs/binder.yaml
@@ -81,6 +81,7 @@ operations:
       name: report-setup
       doc: Set flags from user space.
       attribute-set: cmd
+      flags: [ admin-perm ]

       do:
         request: &params
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 536be42c531e..f6791f5f231a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -6500,13 +6500,6 @@ int binder_nl_report_setup_doit(struct sk_buff *skb, struct genl_info *info)
 	pid = nla_get_u32(info->attrs[BINDER_A_CMD_PID]);
 	flags = nla_get_u32(info->attrs[BINDER_A_CMD_FLAGS]);

-	if (context->report_portid && context->report_portid != portid) {
-		NL_SET_ERR_MSG_FMT(info->extack,
-				   "No permission to set flags from %d",
-				   portid);
-		return -EPERM;
-	}
-
	if (!pid) {
		/* Set the global flags for the whole binder context */
		context->report_flags = flags;
diff --git a/drivers/android/binder_netlink.c b/drivers/android/binder_netlink.c
index ea008f4f3635..6b3d93ff7c5d 100644
--- a/drivers/android/binder_netlink.c
+++ b/drivers/android/binder_netlink.c
@@ -24,7 +24,7 @@ static const struct genl_split_ops binder_nl_ops[] = {
 		.doit		= binder_nl_report_setup_doit,
 		.policy		= binder_report_setup_nl_policy,
 		.maxattr	= BINDER_A_CMD_FLAGS,
-		.flags		= GENL_CMD_CAP_DO,
+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
 	},
 };