On Mon, Apr 21, 2025 at 8:17 AM Simon Horman <horms@xxxxxxxxxx> wrote:
>
> On Wed, Apr 16, 2025 at 05:20:03PM -0700, Li Li wrote:
> > From: Li Li <dualli@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
>
> 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.
>
Thank you for flagging this! Let me double check the lifecycle of t.
> > 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.
>
Indeed, I should use proc->context->name here.
> > + 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;
> > +}
>
> ...
