| 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
| ||
|
Message-ID: <20241022170625.GJ402847@kernel.org>
Date: Tue, 22 Oct 2024 18:06:25 +0100
From: Simon Horman <horms@...nel.org>
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, cmllamas@...gle.com,
surenb@...gle.com, arnd@...db.de, masahiroy@...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 v4 1/1] binder: report txn errors via generic netlink
(genl)
On Mon, Oct 21, 2024 at 12:12:33PM -0700, Li Li wrote:
> From: Li Li <dualli@...gle.com>
>
> 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.
>
> The new binder genl sources and headers are automatically generated from
> the corresponding binder_genl YAML spec. Don't modify them directly.
>
> Signed-off-by: Li Li <dualli@...gle.com>
...
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
...
> @@ -2984,6 +2985,130 @@ static void binder_set_txn_from_error(struct binder_transaction *t, int id,
> binder_thread_dec_tmpref(from);
> }
>
> +/**
> + * binder_find_proc() - set binder report flags
> + * @pid: the target process
> + */
> +static struct binder_proc *binder_find_proc(int pid)
> +{
> + struct binder_proc *proc;
> +
> + mutex_lock(&binder_procs_lock);
> + hlist_for_each_entry(proc, &binder_procs, proc_node) {
> + if (proc->pid == pid) {
> + mutex_unlock(&binder_procs_lock);
> + return proc;
> + }
> + }
> + mutex_unlock(&binder_procs_lock);
> +
> + return NULL;
> +}
> +
> +/**
> + * binder_genl_set_report() - set binder report flags
> + * @proc: the binder_proc calling the ioctl
nit: binder_genl_set_report does not have a proc parameter,
but it does have a context parameter.
> + * @pid: the target process
> + * @flags: the flags to set
> + *
> + * If pid is 0, the flags are applied to the whole binder context.
> + * Otherwise, the flags are applied to the specific process only.
> + */
> +static int binder_genl_set_report(struct binder_context *context, u32 pid, u32 flags)
...
> static int __init init_binder_device(const char *name)
> {
> int ret;
> @@ -6920,6 +7196,11 @@ static int __init init_binder_device(const char *name)
The code above this hunk looks like this:
ret = misc_register(&binder_device->miscdev);
if (ret < 0) {
kfree(binder_device);
return ret;
}
>
> hlist_add_head(&binder_device->hlist, &binder_devices);
>
> + binder_device->context.report_seq = (atomic_t)ATOMIC_INIT(0);
> + ret = binder_genl_init(&binder_device->context.genl_family, name);
> + if (ret < 0)
> + kfree(binder_device);
So I think that binder_device->miscdev needs to be misc_deregister'ed
if we hit this error condition.
> +
> return ret;
Probably adding an unwind ladder like this makes sense (completely untested!):
ret = misc_register(&binder_device->miscdev);
if (ret < 0)
goto err_misc_deregister;
hlist_add_head(&binder_device->hlist, &binder_devices);
binder_device->context.report_seq = (atomic_t)ATOMIC_INIT(0);
ret = binder_genl_init(&binder_device->context.genl_family, name);
if (ret < 0);
goto err_misc_deregister;
return 0;
err_misc_deregister:
misc_deregister(&binder_device->miscdev);
err_free_dev:
kfree(binder_device);
return ret;
...
> diff --git a/drivers/android/binder_genl.h b/drivers/android/binder_genl.h
Perhaps it is because of a different version of net-next,
but with this patch applied on top of the current head commit
13feb6074a9f ("binder: report txn errors via generic netlink (genl)")
I see:
$ ./tools/net/ynl/ynl-regen.sh -f
$ git diff
diff --git a/include/uapi/linux/android/binder_genl.h b/include/uapi/linux/android/binder_genl.h
index ef5289133be5..93e58b370420 100644
--- a/include/uapi/linux/android/binder_genl.h
+++ b/include/uapi/linux/android/binder_genl.h
@@ -3,12 +3,17 @@
/* Documentation/netlink/specs/binder_genl.yaml */
/* YNL-GEN uapi header */
-#ifndef _UAPI_LINUX_BINDER_GENL_H
-#define _UAPI_LINUX_BINDER_GENL_H
+#ifndef _UAPI_LINUX_ANDROID/BINDER_GENL_H
+#define _UAPI_LINUX_ANDROID/BINDER_GENL_H
#define BINDER_GENL_FAMILY_NAME "binder_genl"
#define BINDER_GENL_FAMILY_VERSION 1
+/**
+ * enum binder_genl_flag - Used with "set" and "reply" command below, defining
+ * what kind \ of binder transactions should be reported to the user space \
+ * administration process.
+ */
enum binder_genl_flag {
BINDER_GENL_FLAG_FAILED = 1,
BINDER_GENL_FLAG_DELAYED = 2,
@@ -34,4 +39,4 @@ enum {
BINDER_GENL_CMD_MAX = (__BINDER_GENL_CMD_MAX - 1)
};
-#endif /* _UAPI_LINUX_BINDER_GENL_H */
+#endif /* _UAPI_LINUX_ANDROID/BINDER_GENL_H */
...
--
pw-bot: changes-requested
Powered by blists - more mailing lists