linux-kernel - Re: [PATCH v1] add binder genl for txn report

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <2024081341-egging-custodian-428d@gregkh>
Date: Tue, 13 Aug 2024 07:13:02 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Li Li <dualli@...omium.org>
Cc: dualli@...gle.com, arve@...roid.com, tkjos@...roid.com,
	maco@...roid.com, joel@...lfernandes.org, brauner@...nel.org,
	cmllamas@...gle.com, surenb@...gle.com, arnd@...db.de,
	masahiroy@...nel.org, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org, hridya@...gle.com,
	smoreland@...gle.com, kernel-team@...roid.com
Subject: Re: [PATCH v1] add binder genl for txn report

On Mon, Aug 12, 2024 at 02:18:44PM -0700, Li Li wrote:
> From: Li Li <dualli@...gle.com>

Sorry, but I can not parse your Subject: line at all.  Please use
vowels, we don't have a lack of them :)

Also look at how other patches are formatted for these files to get an
idea of how to create a good subject line.

> Frozen tasks can't process binder transactions, so sync binder
> transactions will fail with BR_FROZEN_REPLY and async binder
> transactions will be queued in the kernel async binder buffer.
> As these queued async transactions accumulates over time, the async
> buffer will eventually be running out, denying all new transactions
> after that with BR_FAILED_REPLY.
> 
> In addition to the above cases, different kinds of binder error codes
> might be returned to the sender. However, the core Linux, or Android,
> system administration process never knows what's actually happening.
> 
> This patch introduces the Linux generic netlink messages into the binder
> driver so that the Linux/Android system administration process can
> listen to important events and take corresponding actions, like stopping
> a broken app from attacking the OS by sending huge amount of spamming
> binder transactions.
> 
> To prevent making the already bloated binder.c even bigger, a new source
> file binder_genl.c is created to host those generic netlink code.

"genl" is a rough abbreviation that is not going to be easy to remember,
what's wrong with "binder_netlink.c"?


> 
> Usage example (user space pseudo code):
> 
> 	// enable binder report from the interested binder context(s)
> 	struct binder_report_info info = {0, BINDER_REPORT_ALL};
> 	ioctl(binder1, BINDER_ENABLE_REPORT, &info);
> 	ioctl(binder2, BINDER_ENABLE_REPORT, &info);
> 
> 	// set optional per-process report, overriding the global one
> 	struct binder_report_info info2;
> 	info2.pid = getpid();
> 	info2.flags = BINDER_REPORT_FAILED | BINDER_REPORT_OVERRIDE;
> 	ioctl(binder2, BINDER_ENABLE_REPORT, &info2);
> 
> 	// open netlink socket
> 	int fd = socket(AF_NETLINK, SOCK_RAW, NETLINK_GENERIC);
> 
> 	// bind netlink socket
> 	bind(fd, struct socketaddr);
> 
> 	// get the family id of binder generic netlink
> 	send(fd, CTRL_CMD_GETFAMILY, CTRL_ATTR_FAMILY_NAME);
> 	int id = recv(CTRL_ATTR_FAMILY_ID);
> 
> 	// register the current process to receive binder reports
> 	send(fd, id, BINDER_GENL_CMD_REGISTER);
> 
> 	// verify registration by reading back the registered pid
> 	recv(fd, BINDER_GENL_ATTR_PID, &pid);
> 
> 	// wait and read all binder reports
> 	while (running) {
> 		struct binder_report report;
> 		recv(fd, BINDER_GENL_ATTR_REPORT, &report);
> 
> 		// process struct binder_report
> 		do_something(&report);
> 	}
> 
> 	// clean up
> 	close(fd);

What userspace code is now going to use this and require it?  How was it
tested?  Where is the test code?  Where is the new user/kernel api that
you created here documented?

You added a new ioctl here as well, why not mention that?  Why is it
needed?  Why not always emit netlink messages?  How do you turn them
off?

And how does this deal with binder namespaces?  Are these messages all
now "global" somehow?  Or are they separated properly per namespace?

thanks,

greg k-h