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

["Tiffany Y. Yang" <ynaffit@google.com>]

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