linux-kernel - Re: [PATCH] binder: remove unneeded size check code

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] [day] [month] [year] [list]

Message-ID: <Y35wg8/lyyam+dfP@google.com>
Date:   Wed, 23 Nov 2022 19:12:03 +0000
From:   Carlos Llamas <cmllamas@...gle.com>
To:     "Jiazi.Li" <jqqlijiazi@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>, arve@...roid.com,
        Todd Kjos <tkjos@...roid.com>,
        Martijn Coenen <maco@...roid.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Christian Brauner <brauner@...nel.org>,
        "Jiazi.Li" <jiazi.li@...nssion.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] binder: remove unneeded size check code

On Tue, Nov 15, 2022 at 08:03:51PM +0800, Jiazi.Li wrote:
> In binder_ioctl function, the legitimacy check of cmd size has been
> done in switch-case code:
> switch (cmd) {
> 	case BINDER_WRITE_READ;//BINDER_WRITE_READ contains size info
> 
> So unneeded do size check in binder_ioctl and binder_ioctl_write_read
> again.
> 
> In the following version of Google GKI:
> 
> Linux version 5.10.110-android12-9-00011-g2c814f559132-ab8969555
> 
> It seems that the compiler has made optimization and has not passed
> cmd parameters to binder_ioctl_write_read:
> <binder_ioctl+628>:  mov     w8, #0x6201                     // #25089
> <binder_ioctl+632>:  movk    w8, #0xc030, lsl #16
> <binder_ioctl+636>:  cmp     w20, w8
> <binder_ioctl+640>:  b.ne    0xffffffda8aa97880 <binder_ioctl+3168>
> <binder_ioctl+644>:  mov     x0, x23 //filp
> <binder_ioctl+648>:  mov     x1, x27 //arg
> <binder_ioctl+652>:  mov     x2, x22 //thread
> <binder_ioctl+656>:  bl      0xffffffda8aa9e6e4 <binder_ioctl_write_read>
> <binder_ioctl+660>:  mov     w26, w0
> 
> Signed-off-by: Jiazi.Li <jiazi.li@...nssion.com>
> ---
>  drivers/android/binder.c | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 880224ec6abb..48e5a3531282 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -5006,20 +5006,14 @@ static __poll_t binder_poll(struct file *filp,
>  	return 0;
>  }
>  
> -static int binder_ioctl_write_read(struct file *filp,
> -				unsigned int cmd, unsigned long arg,
> +static int binder_ioctl_write_read(struct file *filp, unsigned long arg,
>  				struct binder_thread *thread)
>  {
>  	int ret = 0;
>  	struct binder_proc *proc = filp->private_data;
> -	unsigned int size = _IOC_SIZE(cmd);
>  	void __user *ubuf = (void __user *)arg;
>  	struct binder_write_read bwr;
>  
> -	if (size != sizeof(struct binder_write_read)) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
>  	if (copy_from_user(&bwr, ubuf, sizeof(bwr))) {
>  		ret = -EFAULT;
>  		goto out;
> @@ -5296,7 +5290,6 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	int ret;
>  	struct binder_proc *proc = filp->private_data;
>  	struct binder_thread *thread;
> -	unsigned int size = _IOC_SIZE(cmd);
>  	void __user *ubuf = (void __user *)arg;
>  
>  	/*pr_info("binder_ioctl: %d:%d %x %lx\n",
> @@ -5318,7 +5311,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  
>  	switch (cmd) {
>  	case BINDER_WRITE_READ:
> -		ret = binder_ioctl_write_read(filp, cmd, arg, thread);
> +		ret = binder_ioctl_write_read(filp, arg, thread);
>  		if (ret)
>  			goto err;
>  		break;
> @@ -5361,10 +5354,6 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	case BINDER_VERSION: {
>  		struct binder_version __user *ver = ubuf;
>  
> -		if (size != sizeof(struct binder_version)) {
> -			ret = -EINVAL;
> -			goto err;
> -		}
>  		if (put_user(BINDER_CURRENT_PROTOCOL_VERSION,
>  			     &ver->protocol_version)) {
>  			ret = -EINVAL;
> -- 
> 2.17.1
> 

Looks good, thanks!

Acked-by: Carlos Llamas <cmllamas@...gle.com>