Extract non-critical sections from binder_alloc_new_buf_locked() that
don't require holding the alloc->mutex. While we are here, consolidate
the multiple checks for size overflow into a single statement.

Also add a few touchups to follow the coding guidelines.

Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 85 ++++++++++++++++++----------------
 1 file changed, 44 insertions(+), 41 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 27c7163761c4..ed1f52f98b0d 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -368,9 +368,7 @@ static bool debug_low_async_space_locked(struct binder_alloc *alloc, int pid)
 
 static struct binder_buffer *binder_alloc_new_buf_locked(
 				struct binder_alloc *alloc,
-				size_t data_size,
-				size_t offsets_size,
-				size_t extra_buffers_size,
+				size_t size,
 				int is_async,
 				int pid)
 {
@@ -378,39 +376,10 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 	struct binder_buffer *buffer;
 	size_t buffer_size;
 	struct rb_node *best_fit = NULL;
-	size_t size, data_offsets_size;
 	unsigned long has_page_addr;
 	unsigned long end_page_addr;
 	int ret;
 
-	/* Check binder_alloc is fully initialized */
-	if (!binder_alloc_get_vma(alloc)) {
-		binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
-				   "%d: binder_alloc_buf, no vma\n",
-				   alloc->pid);
-		return ERR_PTR(-ESRCH);
-	}
-
-	data_offsets_size = ALIGN(data_size, sizeof(void *)) +
-		ALIGN(offsets_size, sizeof(void *));
-
-	if (data_offsets_size < data_size || data_offsets_size < offsets_size) {
-		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
-				"%d: got transaction with invalid size %zd-%zd\n",
-				alloc->pid, data_size, offsets_size);
-		return ERR_PTR(-EINVAL);
-	}
-	size = data_offsets_size + ALIGN(extra_buffers_size, sizeof(void *));
-	if (size < data_offsets_size || size < extra_buffers_size) {
-		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
-				"%d: got transaction with invalid extra_buffers_size %zd\n",
-				alloc->pid, extra_buffers_size);
-		return ERR_PTR(-EINVAL);
-	}
-
-	/* Pad 0-size buffers so they get assigned unique addresses */
-	size = max(size, sizeof(void *));
-
 	if (is_async &&
 	    alloc->free_async_space < size + sizeof(struct binder_buffer)) {
 		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
@@ -427,13 +396,14 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 		if (size < buffer_size) {
 			best_fit = n;
 			n = n->rb_left;
-		} else if (size > buffer_size)
+		} else if (size > buffer_size) {
 			n = n->rb_right;
-		else {
+		} else {
 			best_fit = n;
 			break;
 		}
 	}
+
 	if (best_fit == NULL) {
 		size_t allocated_buffers = 0;
 		size_t largest_alloc_size = 0;
@@ -511,11 +481,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 	binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
 		     "%d: binder_alloc_buf size %zd got %pK\n",
 		      alloc->pid, size, buffer);
-	buffer->data_size = data_size;
-	buffer->offsets_size = offsets_size;
-	buffer->async_transaction = is_async;
-	buffer->extra_buffers_size = extra_buffers_size;
-	buffer->pid = pid;
+
 	buffer->oneway_spam_suspect = false;
 	if (is_async) {
 		alloc->free_async_space -= size + sizeof(struct binder_buffer);
@@ -533,6 +499,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 			alloc->oneway_spam_detected = false;
 		}
 	}
+
 	return buffer;
 
 err_alloc_buf_struct_failed:
@@ -565,11 +532,47 @@ struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
 					   int pid)
 {
 	struct binder_buffer *buffer;
+	size_t size;
+
+	/* Check binder_alloc is fully initialized */
+	if (!binder_alloc_get_vma(alloc)) {
+		binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
+				   "%d: binder_alloc_buf, no vma\n",
+				   alloc->pid);
+		return ERR_PTR(-ESRCH);
+	}
+
+	size = ALIGN(data_size, sizeof(void *)) +
+		ALIGN(offsets_size, sizeof(void *)) +
+		ALIGN(extra_buffers_size, sizeof(void *));
+
+	if (size < data_size ||
+	    size < offsets_size ||
+	    size < extra_buffers_size) {
+		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
+				   "%d: got transaction with invalid size %zd-%zd-%zd\n",
+				   alloc->pid, data_size, offsets_size,
+				   extra_buffers_size);
+		return ERR_PTR(-EINVAL);
+	}
+
+	/* Pad 0-size buffers so they get assigned unique addresses */
+	size = max(size, sizeof(void *));
 
 	mutex_lock(&alloc->mutex);
-	buffer = binder_alloc_new_buf_locked(alloc, data_size, offsets_size,
-					     extra_buffers_size, is_async, pid);
+	buffer = binder_alloc_new_buf_locked(alloc, size, is_async, pid);
 	mutex_unlock(&alloc->mutex);
+
+	if (IS_ERR(buffer))
+		goto out;
+
+	buffer->data_size = data_size;
+	buffer->offsets_size = offsets_size;
+	buffer->async_transaction = is_async;
+	buffer->extra_buffers_size = extra_buffers_size;
+	buffer->pid = pid;
+
+out:
 	return buffer;
 }
 
-- 
2.42.0.869.gea05f2083d-goog

I found a few issues in this patch:

Carlos Llamas <cmllamas@google.com> writes:
> -	data_offsets_size = ALIGN(data_size, sizeof(void *)) +
> -		ALIGN(offsets_size, sizeof(void *));
> -
> -	if (data_offsets_size < data_size || data_offsets_size < offsets_size) {
> -		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
> -				"%d: got transaction with invalid size %zd-%zd\n",
> -				alloc->pid, data_size, offsets_size);
> -		return ERR_PTR(-EINVAL);
> -	}
> -	size = data_offsets_size + ALIGN(extra_buffers_size, sizeof(void *));
> -	if (size < data_offsets_size || size < extra_buffers_size) {
> -		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
> -				"%d: got transaction with invalid extra_buffers_size %zd\n",
> -				alloc->pid, extra_buffers_size);
> -		return ERR_PTR(-EINVAL);
> -	}
> [snip]
> +	size = ALIGN(data_size, sizeof(void *)) +
> +		ALIGN(offsets_size, sizeof(void *)) +
> +		ALIGN(extra_buffers_size, sizeof(void *));
> +
> +	if (size < data_size ||
> +	    size < offsets_size ||
> +	    size < extra_buffers_size) {
> +		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
> +				   "%d: got transaction with invalid size %zd-%zd-%zd\n",
> +				   alloc->pid, data_size, offsets_size,
> +				   extra_buffers_size);
> +		return ERR_PTR(-EINVAL);
> +	}

Consolidating the overflow check into one if statement like this doesn't
catch all cases of integer overflow. For example, if all three sizes are
9223372036854775816, then the computed size will be 9223372036854775832,
so this would not trigger the overflow check.

Carlos Llamas <cmllamas@google.com> writes:
>  	mutex_unlock(&alloc->mutex);
> +
> +	if (IS_ERR(buffer))
> +		goto out;
> +
> +	buffer->data_size = data_size;
> +	buffer->offsets_size = offsets_size;
> +	buffer->async_transaction = is_async;
> +	buffer->extra_buffers_size = extra_buffers_size;
> +	buffer->pid = pid;

With this change, if there is a concurrent call to
debug_low_async_space_locked, then there is a data race on the
async_transaction field. Similarly for print_binder_buffer.

Perhaps these writes should be moved before the mutex_unlock?

Alice

On Tue, Nov 07, 2023 at 09:08:18AM +0000, Alice Ryhl wrote:
> I found a few issues in this patch:
> 
> Consolidating the overflow check into one if statement like this doesn't
> catch all cases of integer overflow. For example, if all three sizes are
> 9223372036854775816, then the computed size will be 9223372036854775832,
> so this would not trigger the overflow check.

Thanks for pointing this out, you are right.

I don't understand the reasoning behind using size_t for the uapi. It
just made things more complicated than needed. These sizes are much
larger than the maximum buffer size of SZ_4M.

Anyway, I've fixed this for v2.

> 
> Carlos Llamas <cmllamas@google.com> writes:
> >  	mutex_unlock(&alloc->mutex);
> > +
> > +	if (IS_ERR(buffer))
> > +		goto out;
> > +
> > +	buffer->data_size = data_size;
> > +	buffer->offsets_size = offsets_size;
> > +	buffer->async_transaction = is_async;
> > +	buffer->extra_buffers_size = extra_buffers_size;
> > +	buffer->pid = pid;
> 
> With this change, if there is a concurrent call to
> debug_low_async_space_locked, then there is a data race on the
> async_transaction field. Similarly for print_binder_buffer.
> 
> Perhaps these writes should be moved before the mutex_unlock?

Also fixed, thanks!

--
Carlos Llamas