linux-kernel - Re: [PATCH 13/13] android: binder: add function for processing work nodes in binder_thread

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: <20150529115616.GK28762@mwanda>
Date:	Fri, 29 May 2015 14:56:16 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Riley Andrews <riandrews@...roid.com>
Cc:	linux-kernel@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Arve Hjønnevåg <arve@...roid.com>,
	devel@...verdev.osuosl.org
Subject: Re: [PATCH 13/13] android: binder: add function for processing work
 nodes in binder_thread_read

On Thu, May 28, 2015 at 04:08:31PM -0700, Riley Andrews wrote:
> -done:
> +static int binder_thread_read(struct binder_proc *proc,
> +			      struct binder_thread *thread,
> +			      binder_uintptr_t binder_buffer, size_t size,
> +			      binder_size_t *consumed, int non_block)
> +{
> +	void __user *buffer = (void __user *)(uintptr_t)binder_buffer;
> +	void __user *ptr = buffer + *consumed;
> +	void __user *end = buffer + size;
> +	bool wait_for_proc_work;
> +
> +	int ret = 0;
> +
> +	if (*consumed == 0) {
> +		if (put_user(BR_NOOP, (uint32_t __user *)ptr))
> +			return -EFAULT;
> +		ptr += sizeof(uint32_t);
> +	}
> +
> +	do {
> +		if (thread->return_error != BR_OK) {
> +			ret =  binder_handle_thread_error(thread, &ptr, end);
> +			if (ret < 0)
> +				return ret;
> +			break;
> +		}
> +		if (!thread->transaction_stack && list_empty(&thread->todo))
> +			wait_for_proc_work = true;
> +		else
> +			wait_for_proc_work = false;
> +
> +		ret = binder_wait_for_work(thread, non_block,
> +					   wait_for_proc_work);
> +		if (ret)
> +			return ret;
> +
> +		ret = binder_thread_read_do_work(thread, wait_for_proc_work,
> +						 buffer, end, &ptr);
> +		if (ret)
> +			return ret;
> +	} while ((ptr - buffer == 4) &&
> +		 !(thread->looper & BINDER_LOOPER_STATE_NEED_RETURN) &&
> +		 ((end - ptr) >= sizeof(struct binder_transaction_data) + 4));

"end" and "buffer" don't change so we could move check:

		((end - ptr) >= sizeof(struct binder_transaction_data) + 4)

to the start of the function.  I may have missed something because I'm
not terribly familiar with this code.

I don't really like the way this condition is written because if "ptr"
were greater than "end" it would be true.  This seems like something
that might happen.  Pass in bwr.read_size = 1. When we do the first
ptr += sizeof(uint32_t); then "end" is less than "ptr".

This condition was there in the original code as well so it's not
something the patch introduced but it worries me every time I look at
it, even if it turns out that it's not a problem.

Please write it like:

	(ptr + sizeof(struct binder_transaction_data) + 4 <= end)

or whatever so that we don't have to think about negative numbers.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/