linux-kernel - Re: [PATCH 10/21] binder: do unlocked work in binder_alloc_new

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

Message-ID: <20231107090818.258621-1-aliceryhl@google.com>
Date:   Tue,  7 Nov 2023 09:08:18 +0000
From:   Alice Ryhl <aliceryhl@...gle.com>
To:     Carlos Llamas <cmllamas@...gle.com>
Cc:     "Arve Hjønnevåg" <arve@...roid.com>,
        Christian Brauner <brauner@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Joel Fernandes <joel@...lfernandes.org>,
        kernel-team@...roid.com, linux-kernel@...r.kernel.org,
        Martijn Coenen <maco@...roid.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Todd Kjos <tkjos@...roid.com>
Subject: Re: [PATCH 10/21] binder: do unlocked work in binder_alloc_new_buf()

I found a few issues in this patch:

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