[PATCH v2 1/3] Add decoding for binder ioctls

[Antoine Damhet]

Le Mon, 20 Jun 2016 15:07:52 +0300,
"Dmitry V. Levin" <ldv at altlinux.org> a écrit :

> On Sat, Jun 18, 2016 at 07:13:54PM +0200, Antoine Damhet wrote:
> [...]
> > +static int
> > +decode_binder_commands_buffer(struct tcb *tcp, uintptr_t buffer,
> > size_t pos,
> > +		size_t size, const struct xlat *x, const char *str)
> > +{
> > +	if (size < sizeof(uint32_t)) {
> > +		tprints("[]");
> > +		return 0;
> > +	}  
> 
> Wouldn't it be better to print the address of buffer instead?
> 

Will do.

> > +
> > +	if (abbrev(tcp)) {
> > +		tprints("[...]");
> > +		return 0;
> > +	}
> > +
> > +	uint32_t type;
> > +	tprints("[");
> > +
> > +	goto print_one_commands_buffer;
> > +	while (pos + sizeof(uint32_t) <= size) {  
> 
> Why the check is skipped for the first time?
> 

I forgot that pos wasn't initialized a 0.

> > +		tprints(", ");
> > +
> > +print_one_commands_buffer:
> > +		if (umove(tcp, buffer + pos, &type))  
> 
> What's going to be fetched when buffer + pos overflows?
> 
> > +			return 1;  
> 
> What's going to be printed when this umove call fails?
> 

I will print ']'.

> > +		if (_IOC_SIZE(type) > 0) {
> > +			tprints("[");
> > +			printxval(x, type, str);
> > +			tprints(", ");
> > +			if (pos + sizeof(type) + _IOC_SIZE(type)
> > <= size)
> > +				printstr(tcp, buffer + pos +
> > sizeof(type),
> > +						_IOC_SIZE(type));
> > +			tprints("]");  
> 
> What's going to be printed when this condition is false?
> 

What should I print ?

> > +		} else
> > +			printxval(x, type, str);
> > +		pos += sizeof(uint32_t) + _IOC_SIZE(type);  
> 
> What's going to happen when pos overflows?
> 

The umove should have failed before but should I still check it ?

> > +	}
> > +
> > +	tprints("]");
> > +	return 0;
> > +}
> > +
> > +static int
> > +decode_binder_write_read(struct tcb *tcp, const long addr)
> > +{
> > +	struct binder_write_read wr;
> > +
> > +	if (entering(tcp)) {
> > +		tprints(", ");
> > +		if (umove_or_printaddr(tcp, addr, &wr))
> > +			return RVAL_DECODED | 1;
> > +
> > +		tprintf("{write_size=%" PRIu64 ",
> > write_consumed=%" PRIu64
> > +				", write_buffer=",
> > +				(uint64_t)wr.write_size,
> > +				(uint64_t)wr.write_consumed);
> > +		if (decode_binder_commands_buffer(tcp,
> > wr.write_buffer,
> > +					wr.write_consumed,
> > wr.write_size,
> > +					binder_driver_commands,
> > "BC_???")) {  
> 
> Does the kernel take write_consumed into account?
> If not, shouldn't 0 be passed to decode_binder_commands_buffer
> instead, and what's the use of printing write_consumed on entering?
> 

Yes, the kernel take write_consumed into account.

> > +			tprints("}");
> > +			return RVAL_DECODED | 1;
> > +		}
> > +
> > +		tprintf(", read_size=%" PRIu64 ", read_consumed=%"
> > PRIu64 "}",
> > +				(uint64_t)wr.read_size,
> > +				(uint64_t)wr.read_consumed);  
> 
> If read_consumed is write only, what's the use of printing it on
> entering?
> 

It's not but I'm not sure how I should keep read_consumed between
entering and exiting.

> > +		return 0;
> > +	}
> > +
> > +	if (syserror(tcp) || umove(tcp, addr, &wr))
> > +		return RVAL_DECODED | 1;
> > +
> > +	tprints(" => ");
> > +
> > +	tprintf("{write_size=%" PRIu64 ", write_consumed=%" PRIu64
> > +			", read_size=%" PRIu64 ", read_consumed=%"
> > PRIu64
> > +			", read_buffer=",
> > +			(uint64_t)wr.write_size,
> > +			(uint64_t)wr.write_consumed,
> > +			(uint64_t)wr.read_size,
> > +			(uint64_t)wr.read_consumed);  
> 
> If write_size and read_size are read only, what's the use of printing
> them on exiting?
> 

I won't print them anymore.

> [...]
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -440,6 +440,24 @@ AC_CHECK_HEADERS([linux/bpf.h], [
> >  	fi
> >  ])
> >  
> > +AC_CHECK_HEADERS([linux/android/binder.h], [[ #include
> > <sys/types.h> +#include <linux/android/binder.h>])
> > +        AC_CHECK_DECLS(m4_normalize([BC_TRANSACTION, BC_REPLY,
> > BC_ACQUIRE_RESULT,
> > +	                        BC_FREE_BUFFER, BC_INCREFS,
> > BC_ACQUIRE,
> > +	                        BC_RELEASE, BC_DECREFS,
> > BC_INCREFS_DONE,
> > +	                        BC_ACQUIRE_DONE,
> > BC_ATTEMPT_ACQUIRE,
> > +	                        BC_REGISTER_LOOPER,
> > BC_ENTER_LOOPER,
> > +	                        BC_EXIT_LOOPER,
> > BC_REQUEST_DEATH_NOTIFICATION,
> > +	                        BC_CLEAR_DEATH_NOTIFICATION,
> > BC_DEAD_BINDER_DONE,
> > +	                        BR_ERROR, BR_OK, BR_TRANSACTION,
> > BR_REPLY,
> > +	                        BR_ACQUIRE_RESULT, BR_DEAD_REPLY,
> > +	                        BR_TRANSACTION_COMPLETE,
> > BR_INCREFS, BR_ACQUIRE,
> > +	                        BR_RELEASE, BR_DECREFS,
> > BR_ATTEMPT_ACQUIRE,
> > +	                        BR_NOOP, BR_SPAWN_LOOPER,
> > BR_FINISHED,
> > +	                        BR_DEAD_BINDER,
> > BR_CLEAR_DEATH_NOTIFICATION_DONE,
> > +	                        BR_FAILED_REPLY]),,,[ #include
> > <sys/types.h>  
> 
> Please list these constants sorted in one column like in other places.
> 

Will do.

> > +#include <linux/android/binder.h>])])
> > +  
> 
> I'm not quite sure what's expected from defined(__ANDROID__) in
> binder.c and whether this check is consistent with conditions where
> <linux/android/binder.h> is not available but defined(__ANDROID__) is
> true.
> 
> 

On android, the header is located at <linux/binder.h> and since it's a
vital component in the system so it will be there.

Regards,

-- 
Antoine 'xdbob' Damhet
LSE - System & Security
ACDC/EPITA 2018
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: Signature digitale OpenPGP
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20160622/0f3fa008/attachment.bin>