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

[Dmitry V. Levin]

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?

> +
> +	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?

> +		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?

> +		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?

> +		} else
> +			printxval(x, type, str);
> +		pos += sizeof(uint32_t) + _IOC_SIZE(type);

What's going to happen when pos overflows?

> +	}
> +
> +	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?

> +			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?

> +		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?

[...]
> --- 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.

> +#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.


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20160620/f5d9131c/attachment.bin>