From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1755700AbbE2L4k (ORCPT <rfc822;w@1wt.eu>);
	Fri, 29 May 2015 07:56:40 -0400
Received: from userp1040.oracle.com ([156.151.31.81]:24392 "EHLO
	userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751485AbbE2L4d (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 29 May 2015 07:56:33 -0400
Date: Fri, 29 May 2015 14:56:16 +0300
From: Dan Carpenter <dan.carpenter@oracle.com>
To: Riley Andrews <riandrews@android.com>
Cc: linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Arve =?iso-8859-1?B?SGr4bm5lduVn?= <arve@android.com>,
        devel@driverdev.osuosl.org
Subject: Re: [PATCH 13/13] android: binder: add function for processing work
 nodes in binder_thread_read
Message-ID: <20150529115616.GK28762@mwanda>
References: <1432854511-33320-1-git-send-email-riandrews@android.com>
 <1432854511-33320-14-git-send-email-riandrews@android.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1432854511-33320-14-git-send-email-riandrews@android.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
X-Source-IP: aserv0021.oracle.com [141.146.126.233]
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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