| 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: <20241204183550.6e9d703f@kernel.org>
Date: Wed, 4 Dec 2024 18:35:50 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Li Li <dualli@...omium.org>
Cc: dualli@...gle.com, corbet@....net, davem@...emloft.net,
edumazet@...gle.com, 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, 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 net-next v8 2/2] binder: report txn errors via generic
netlink
On Wed, 13 Nov 2024 11:32:39 -0800 Li Li wrote:
> +/**
> + * 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
> + * @context: the binder context to set the flags
> + * @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)
> +{
> + struct binder_proc *proc;
> +
> + if (flags != (flags & (BINDER_GENL_FLAG_OVERRIDE
> + | BINDER_GENL_FLAG_FAILED
> + | BINDER_GENL_FLAG_DELAYED
> + | BINDER_GENL_FLAG_SPAM))) {
> + pr_err("Invalid binder report flags: %u\n", flags);
> + return -EINVAL;
no need, netlink already checks that only bits from the flags are used:
vvvvvvvvvvvvvvvvvvvvvvvvvvvvv
+ [BINDER_GENL_A_CMD_FLAGS] = NLA_POLICY_MASK(NLA_U32, 0xf),
> + }
> +
> + if (!pid) {
> + /* Set the global flags for the whole binder context */
> + context->report_flags = flags;
> + } else {
> + /* Set the per-process flags */
> + proc = binder_find_proc(pid);
> + if (!proc) {
> + pr_err("Invalid binder report pid %u\n", pid);
> + return -EINVAL;
> + }
> +
> + proc->report_flags = flags;
> + }
> +
> + return 0;
> +}
> +static void binder_genl_send_report(struct binder_context *context, u32 err,
> + u32 pid, u32 tid, u32 to_pid, u32 to_tid,
> + u32 reply,
> + struct binder_transaction_data *tr)
> +{
> + int payload;
> + int ret;
> + struct sk_buff *skb;
> + void *hdr;
> +
> + trace_binder_send_report(context->name, err, pid, tid, to_pid, to_tid,
> + reply, tr);
> +
> + payload = nla_total_size(strlen(context->name) + 1) +
> + nla_total_size(sizeof(u32)) * (BINDER_GENL_A_REPORT_MAX - 1);
> + skb = genlmsg_new(payload + GENL_HDRLEN, GFP_KERNEL);
genlmsg_new() adds the GENL_HDRLEN already
> + if (!skb) {
> + pr_err("Failed to alloc binder genl message\n");
> + return;
> + }
> +
> + hdr = genlmsg_put(skb, 0, atomic_inc_return(&context->report_seq),
> + &binder_genl_nl_family, 0, BINDER_GENL_CMD_REPORT);
> + if (!hdr)
> + goto free_skb;
> +
> + if (nla_put_string(skb, BINDER_GENL_A_REPORT_CONTEXT, context->name) ||
> + nla_put_u32(skb, BINDER_GENL_A_REPORT_ERR, err) ||
> + nla_put_u32(skb, BINDER_GENL_A_REPORT_FROM_PID, pid) ||
> + nla_put_u32(skb, BINDER_GENL_A_REPORT_FROM_TID, tid) ||
> + nla_put_u32(skb, BINDER_GENL_A_REPORT_TO_PID, to_pid) ||
> + nla_put_u32(skb, BINDER_GENL_A_REPORT_TO_TID, to_tid) ||
> + nla_put_u32(skb, BINDER_GENL_A_REPORT_REPLY, reply) ||
> + nla_put_u32(skb, BINDER_GENL_A_REPORT_FLAGS, tr->flags) ||
> + nla_put_u32(skb, BINDER_GENL_A_REPORT_CODE, tr->code) ||
> + nla_put_u32(skb, BINDER_GENL_A_REPORT_DATA_SIZE, tr->data_size))
> + goto cancel_skb;
> +
> + genlmsg_end(skb, hdr);
> +
> + ret = genlmsg_unicast(&init_net, skb, context->report_portid);
> + if (ret < 0)
> + pr_err("Failed to send binder genl message to %d: %d\n",
> + context->report_portid, ret);
> + return;
> +
> +cancel_skb:
> + pr_err("Failed to add report attributes to binder genl message\n");
> + genlmsg_cancel(skb, hdr);
> +free_skb:
> + pr_err("Free binder genl report message on error\n");
> + nlmsg_free(skb);
> +}
> +/**
> + * binder_genl_nl_set_doit() - .doit handler for BINDER_GENL_CMD_SET
> + * @skb: the metadata struct passed from netlink driver
> + * @info: the generic netlink struct passed from netlink driver
> + *
> + * Implements the .doit function to process binder genl commands.
> + */
> +int binder_genl_nl_set_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> + int payload;
> + int portid;
> + u32 pid;
> + u32 flags;
> + void *hdr;
> + struct binder_device *device;
> + struct binder_context *context = NULL;
> +
> + if (GENL_REQ_ATTR_CHECK(info, BINDER_GENL_A_CMD_CONTEXT) ||
> + GENL_REQ_ATTR_CHECK(info, BINDER_GENL_A_CMD_PID) ||
> + GENL_REQ_ATTR_CHECK(info, BINDER_GENL_A_CMD_FLAGS))
> + return -EINVAL;
> +
> + hlist_for_each_entry(device, &binder_devices, hlist) {
> + if (!nla_strcmp(info->attrs[BINDER_GENL_A_CMD_CONTEXT],
> + device->context.name)) {
> + context = &device->context;
> + break;
> + }
> + }
> +
> + if (!context) {
> + NL_SET_ERR_MSG(info->extack, "Unknown binder context\n");
> + return -EINVAL;
> + }
> +
> + portid = nlmsg_hdr(skb)->nlmsg_pid;
> + pid = nla_get_u32(info->attrs[BINDER_GENL_A_CMD_PID]);
> + flags = nla_get_u32(info->attrs[BINDER_GENL_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\n",
> + portid);
> + return -EPERM;
> + }
> +
> + if (binder_genl_set_report(context, pid, flags) < 0) {
> + pr_err("Failed to set report flags %u for %u\n", flags, pid);
> + return -EINVAL;
> + }
> +
> + payload = nla_total_size(sizeof(pid)) + nla_total_size(sizeof(flags));
> + skb = genlmsg_new(payload + GENL_HDRLEN, GFP_KERNEL);
> + if (!skb) {
> + pr_err("Failed to alloc binder genl reply message\n");
> + return -ENOMEM;
no need for error messages on allocation failures, kernel will print an
OOM report
> + }
> +
> + hdr = genlmsg_iput(skb, info);
> + if (!hdr)
> + goto free_skb;
> +
> + if (nla_put_string(skb, BINDER_GENL_A_CMD_CONTEXT, context->name) ||
Have you counted strlen(context->name) to payload?
TBH for small messages counting payload size is probably an overkill,
you can use NLMSG_GOODSIZE as the size of the skb.
> + nla_put_u32(skb, BINDER_GENL_A_CMD_PID, pid) ||
> + nla_put_u32(skb, BINDER_GENL_A_CMD_FLAGS, flags))
> + goto cancel_skb;
> +
> + genlmsg_end(skb, hdr);
> +
> + if (genlmsg_reply(skb, info)) {
> + pr_err("Failed to send binder genl reply message\n");
> + return -EFAULT;
> + }
> +
> + if (!context->report_portid)
> + context->report_portid = portid;
Is there any locking required?
> + return 0;
> +
> +cancel_skb:
> + pr_err("Failed to add reply attributes to binder genl message\n");
> + genlmsg_cancel(skb, hdr);
> +free_skb:
> + pr_err("Free binder genl reply message on error\n");
> + nlmsg_free(skb);
> + return -EMSGSIZE;
> +}
Powered by blists - more mailing lists