linux-kernel - Re: [PATCH 1/4] binder: introduce BINDER_SET_PROC

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: <20240418083447.3877366-1-aliceryhl@google.com>
Date: Thu, 18 Apr 2024 08:34:47 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: cmllamas@...gle.com
Cc: aliceryhl@...gle.com, arve@...roid.com, brauner@...nel.org, 
	gregkh@...uxfoundation.org, joel@...lfernandes.org, kernel-team@...roid.com, 
	linux-kernel@...r.kernel.org, maco@...roid.com, surenb@...gle.com, 
	tkjos@...roid.com
Subject: Re: [PATCH 1/4] binder: introduce BINDER_SET_PROC_FLAGS ioctl

Carlos Llamas <cmllamas@...gle.com> writes:
> This new ioctl enables userspace to control the individual behavior of
> the 'struct binder_proc' instance via flags. The driver validates and
> returns the supported subset. Some existing ioctls are migrated to use
> these flags in subsequent commits.
> 
> Suggested-by: Arve Hjønnevåg <arve@...roid.com>
> Signed-off-by: Carlos Llamas <cmllamas@...gle.com>
> ---
>  drivers/android/binder.c            | 25 +++++++++++++++++++++++++
>  drivers/android/binder_internal.h   |  4 +++-
>  include/uapi/linux/android/binder.h |  6 ++++++
>  3 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index bad28cf42010..e0d193bfb237 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -5334,6 +5334,26 @@ static int binder_ioctl_get_extended_error(struct binder_thread *thread,
>  	return 0;
>  }
>  
> +static int binder_ioctl_set_proc_flags(struct binder_proc *proc,
> +				       u32 __user *user)
> +{
> +	u32 flags;
> +
> +	if (get_user(flags, user))
> +		return -EFAULT;
> +
> +	binder_inner_proc_lock(proc);
> +	flags &= PF_SUPPORTED_FLAGS_MASK;
> +	proc->flags = flags;
> +	binder_inner_proc_unlock(proc);
> +
> +	/* confirm supported flags with user */
> +	if (put_user(flags, user))
> +		return -EFAULT;
> +
> +	return 0;
> +}

I'm just thinking out loud here, but is this the best API for this
ioctl? Using this API, if I want to toggle the oneway-spam-detection
flag, then I can't do so without knowing the value of all other flags,
and I also need to synchronize all calls to this ioctl.

That's fine for the current use-case where these flags are only set
during startup, but are we confident that no future flag will be toggled
while a process is active?

How about these alternatives?

1. Userspace passes two masks, one containing bits to set, and another
   containing bits to unset. Userspace returns new value of flags. (If
   the same bit is set in both masks, we can fail with EINVAL.)

2. Compare and swap. Userspace passes the expected previous value and
   the desired new value. The kernel returns the actual previous value
   and updates it only if userspace gave the right previous value.

3. Set or unset only. Userspace passes a boolean and a mask. Boolean
   determines whether userspace wants to set or unset the bits set in
   the mask.

I don't know what the usual kernel convention is for this kind of
ioctl, so I'm happy with whatever you all think is best.

Alice