| 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: <20250421151713.GP2789685@horms.kernel.org>
Date: Mon, 21 Apr 2025 16:17:13 +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, omosnace@...hat.com, shuah@...nel.org,
arnd@...db.de, masahiroy@...nel.org, bagasdotme@...il.com,
tweek@...gle.com, paul@...l-moore.com, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org, netdev@...r.kernel.org,
selinux@...r.kernel.org, linux-security-module@...r.kernel.org,
hridya@...gle.com, smoreland@...gle.com, ynaffit@...gle.com,
kernel-team@...roid.com
Subject: Re: [PATCH RESEND v17 2/3] binder: report txn errors via generic
netlink
On Wed, Apr 16, 2025 at 05:20:03PM -0700, Li Li wrote:
> From: Li Li <dualli@...gle.com>
>
> Introduce generic netlink messages into the binder driver so that the
> Linux/Android system administration processes 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 binder netlink sources and headers are automatically generated from
> the corresponding binder netlink YAML spec. Don't modify them directly.
>
> Signed-off-by: Li Li <dualli@...gle.com>
Hi Li Li,
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
...
> static void binder_transaction(struct binder_proc *proc,
> struct binder_thread *thread,
> struct binder_transaction_data *tr, int reply,
> @@ -3683,10 +3764,14 @@ static void binder_transaction(struct binder_proc *proc,
> return_error_line = __LINE__;
> goto err_copy_data_failed;
> }
> - if (t->buffer->oneway_spam_suspect)
> + if (t->buffer->oneway_spam_suspect) {
> tcomplete->type = BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT;
> - else
> + if (binder_netlink_enabled(proc, BINDER_FLAG_SPAM))
> + binder_netlink_report(context, BR_ONEWAY_SPAM_SUSPECT,
> + reply, t);
> + } else {
> tcomplete->type = BINDER_WORK_TRANSACTION_COMPLETE;
> + }
> t->work.type = BINDER_WORK_TRANSACTION;
>
> if (reply) {
> @@ -3736,8 +3821,12 @@ static void binder_transaction(struct binder_proc *proc,
> * process and is put in a pending queue, waiting for the target
> * process to be unfrozen.
> */
> - if (return_error == BR_TRANSACTION_PENDING_FROZEN)
> + if (return_error == BR_TRANSACTION_PENDING_FROZEN) {
> tcomplete->type = BINDER_WORK_TRANSACTION_PENDING;
> + if (binder_netlink_enabled(proc, BINDER_FLAG_ASYNC_FROZEN))
> + binder_netlink_report(context, return_error,
> + reply, t);
> + }
> binder_enqueue_thread_work(thread, tcomplete);
> if (return_error &&
> return_error != BR_TRANSACTION_PENDING_FROZEN)
> @@ -3799,6 +3888,10 @@ static void binder_transaction(struct binder_proc *proc,
The code preceding this hunk looks like this:
err_alloc_tcomplete_failed:
if (trace_binder_txn_latency_free_enabled())
binder_txn_latency_free(t);
kfree(t);
binder_stats_deleted(BINDER_STAT_TRANSACTION);
err_alloc_t_failed:
err_bad_todo_list:
err_bad_call_stack:
err_empty_call_stack:
err_dead_binder:
err_invalid_target_handle:
if (target_node) {
binder_dec_node(target_node, 1, 0);
binder_dec_node_tmpref(target_node);
}
1. The labels err_bad_todo_list, err_bad_call_stack,
err_empty_call_stack, and err_invalid_target_handle may
be jumped to before t is initialised.
2. In the err_alloc_tcomplete_failed label t is kfree'd.
However, the call to binder_netlink_report below will dereference t.
Flagged by Smatch.
> binder_dec_node_tmpref(target_node);
> }
>
> + if (binder_netlink_enabled(proc, BINDER_FLAG_FAILED))
> + binder_netlink_report(context, return_error,
> + reply, t);
> +
> binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
> "%d:%d transaction %s to %d:%d failed %d/%d/%d, code %u size %lld-%lld line %d\n",
> proc->pid, thread->pid, reply ? "reply" :
...
> +/**
> + * binder_nl_report_setup_doit() - netlink .doit handler
> + * @skb: the metadata struct passed from netlink driver
> + * @info: the generic netlink struct passed from netlink driver
> + *
> + * Implements the .doit function to process binder netlink commands.
> + */
> +int binder_nl_report_setup_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> + struct binder_context *context = NULL;
> + struct binder_device *device;
> + struct binder_proc *proc;
> + u32 flags, pid;
> + bool found;
> + void *hdr;
> + int ret;
> +
> + ret = security_binder_setup_report(current_cred());
> + if (ret < 0) {
> + NL_SET_ERR_MSG(info->extack, "Permission denied");
> + return ret;
> + }
> +
> + if (nla_len(info->attrs[BINDER_A_CMD_CONTEXT])) {
> + /* Search the specified binder context */
> + hlist_for_each_entry(device, &binder_devices, hlist) {
> + if (!nla_strcmp(info->attrs[BINDER_A_CMD_CONTEXT],
> + device->context.name)) {
> + context = &device->context;
> + break;
> + }
> + }
> +
> + if (!context) {
> + NL_SET_ERR_MSG(info->extack, "Invalid binder context");
> + return -EINVAL;
> + }
> + }
> +
> + pid = nla_get_u32(info->attrs[BINDER_A_CMD_PID]);
> + flags = nla_get_u32(info->attrs[BINDER_A_CMD_FLAGS]);
> +
> + if (!pid) {
> + if (!context) {
> + NL_SET_ERR_MSG(info->extack,
> + "Invalid binder context and pid");
> + return -EINVAL;
> + }
> +
> + /* Set the global flags for the whole binder context */
> + context->report_flags = flags;
> + } else {
> + /* Set the per-process flags */
> + found = false;
> + mutex_lock(&binder_procs_lock);
> + hlist_for_each_entry(proc, &binder_procs, proc_node) {
> + if (proc->pid == pid
> + && (proc->context == context || !context)) {
> + proc->report_flags = flags;
> + found = true;
> + }
> + }
> + mutex_unlock(&binder_procs_lock);
> +
> + if (!found) {
> + NL_SET_ERR_MSG_FMT(info->extack,
> + "Invalid binder report pid %u",
> + pid);
> + return -EINVAL;
> + }
> + }
Within the above conditions it is assumed that context may be NULL.
> +
> + skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
> + if (!skb) {
> + pr_err("Failed to alloc binder netlink reply message\n");
> + return -ENOMEM;
> + }
> +
> + hdr = genlmsg_iput(skb, info);
> + if (!hdr)
> + goto free_skb;
> +
> + if (nla_put_string(skb, BINDER_A_CMD_CONTEXT, context->name) ||
But here context is dereferenced unconditionally.
This does not seem consistent.
Flagged by Smatch.
> + nla_put_u32(skb, BINDER_A_CMD_PID, pid) ||
> + nla_put_u32(skb, BINDER_A_CMD_FLAGS, flags))
> + goto cancel_skb;
> +
> + genlmsg_end(skb, hdr);
> +
> + if (genlmsg_reply(skb, info)) {
> + pr_err("Failed to send binder netlink reply message\n");
> + return -EFAULT;
> + }
> +
> + return 0;
> +
> +cancel_skb:
> + pr_err("Failed to add reply attributes to binder netlink message\n");
> + genlmsg_cancel(skb, hdr);
> +free_skb:
> + pr_err("Free binder netlink reply message on error\n");
> + nlmsg_free(skb);
> + ret = -EMSGSIZE;
> +
> + return ret;
> +}
...
Powered by blists - more mailing lists