Re: [PATCH 10/21] binder: do unlocked work in binder_alloc_new_buf()

[Alice Ryhl]

I found a few issues in this patch:

Carlos Llamas <cmllamas@xxxxxxxxxx> 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@xxxxxxxxxx> 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