netdev - Re: [PATCH v4 1/1] binder: report txn errors via generic netlink (genl)

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: <CANBPYPh97difTKrD=a6A-0sOVwZS28K=BvH37-62Z5FUO+kNUg@mail.gmail.com>
Date: Tue, 22 Oct 2024 11:11:13 -0700
From: Li Li <dualli@...omium.org>
To: Simon Horman <horms@...nel.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 Tue, Oct 22, 2024 at 10:06 AM Simon Horman <horms@...nel.org> wrote:
>
> On Mon, Oct 21, 2024 at 12:12:33PM -0700, Li Li wrote:
> > + * @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;
>

Good catch! Will fix it in the next revision.

> ...
>
> > 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 */

The patch was based on the top of Linus's tree instead of net-next.
I'll investigate this difference.