linux-kernel - Re: [PATCH v2] binder: use buffer offsets in debug logs

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: <dbx8v7ryym9o.fsf@ynaffit-start.c.googlers.com>
Date: Tue, 25 Mar 2025 00:41:39 +0000
From: "Tiffany Y. Yang" <ynaffit@...gle.com>
To: Carlos Llamas <cmllamas@...gle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,  Arve Hjønnevåg
 <arve@...roid.com>,  Todd Kjos <tkjos@...roid.com>,  Martijn Coenen
 <maco@...roid.com>,  Joel Fernandes <joel@...lfernandes.org>,  Christian
 Brauner <brauner@...nel.org>,  Suren Baghdasaryan <surenb@...gle.com>,
  linux-kernel@...r.kernel.org,  kernel-team@...roid.com
Subject: Re: [PATCH v2] binder: use buffer offsets in debug logs

Carlos Llamas <cmllamas@...gle.com> writes:

> On Mon, Mar 24, 2025 at 06:07:18PM +0000, Tiffany Y. Yang wrote:
>> Identify buffer addresses using vma offsets instead of full user
>> addresses in debug logs.
>> 
>> Signed-off-by: Tiffany Y. Yang <ynaffit@...gle.com>
>> ---
>>  drivers/android/binder.c | 31 ++++++++++++++++---------------
>>  1 file changed, 16 insertions(+), 15 deletions(-)
>> 
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index d1aa6d24450a..994ae205aa07 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -3261,20 +3261,20 @@ static void binder_transaction(struct binder_proc *proc,
>>  
>>  	if (reply)
>>  		binder_debug(BINDER_DEBUG_TRANSACTION,
>> -			     "%d:%d BC_REPLY %d -> %d:%d, data %016llx-%016llx size %lld-%lld-%lld\n",
>> +			     "%d:%d BC_REPLY %d -> %d:%d, buffer offset %lx-%lx size %lld-%lld-%lld\n",
>>  			     proc->pid, thread->pid, t->debug_id,
>>  			     target_proc->pid, target_thread->pid,
>> -			     (u64)tr->data.ptr.buffer,
>> -			     (u64)tr->data.ptr.offsets,
>> +			     (unsigned long)tr->data.ptr.buffer - proc->alloc.buffer,
>> +			     (unsigned long)tr->data.ptr.offsets - proc->alloc.buffer,
>
> These could be pointers to anywhere in user memory, not necessarily the
> alloc->buffer. So there will be cases where this substraction doesn't
> make sense. However, you are correct that we shouldn't log these addrs
> so maybe just don't? wdyt?
>

Ah, in that case I think it makes sense to remove them here. What
do you think about printing the full buffer and offsets values in cases
where we would print a binder_user_error or binder_transaction_error
instead. Ideally, I would try to limit this to cases when the data or
offsets ptr is invalid / copy would fail. Ostensibly this wouldn't
reveal dangerous information about the user address space because the
print statements would only happen when the data wasn't where it was
supposed to be and it would help with debugging, but I'm not sure if
this line of thought makes sense...

>>  			     (u64)tr->data_size, (u64)tr->offsets_size,
>>  			     (u64)extra_buffers_size);
>>  	else
>>  		binder_debug(BINDER_DEBUG_TRANSACTION,
>> -			     "%d:%d BC_TRANSACTION %d -> %d - node %d, data %016llx-%016llx size %lld-%lld-%lld\n",
>> +			     "%d:%d BC_TRANSACTION %d -> %d - node %d, buffer offset %lx-%lx size %lld-%lld-%lld\n",
>>  			     proc->pid, thread->pid, t->debug_id,
>>  			     target_proc->pid, target_node->debug_id,
>> -			     (u64)tr->data.ptr.buffer,
>> -			     (u64)tr->data.ptr.offsets,
>> +			     (unsigned long)tr->data.ptr.buffer - proc->alloc.buffer,
>> +			     (unsigned long)tr->data.ptr.offsets - proc->alloc.buffer,
> same here.
>
> Regards,