Re: [PATCH 1/3] Add decoding for binder ioctls

[Dmitry V. L. <ld...@al...>]

On Sat, May 28, 2016 at 10:40:26PM +0200, Antoine Damhet wrote:
> This patch introduce decoding for binder ioctls including the command
> and return protocol from BINDER_WRITE_READ.

Nice work.  I have some comments, though.

> --- /dev/null
> +++ b/binder.c
> @@ -0,0 +1,211 @@
> +#include "defs.h"
> +
> +#if defined(HAVE_LINUX_ANDROID_BINDER_H) || defined(__ANDROID__)
> +
> +#include <linux/ioctl.h>
> +
> +#if SIZEOF_LONG == 4
> +# define BINDER_IPC_32BIT
> +#endif

As the kernel seems to support both 32bit and 64bit versions of the
protocol, I'd like to see a comment explaining this choice.

> +#ifdef HAVE_LINUX_ANDROID_BINDER_H
> +# include <linux/android/binder.h>
> +#else
> +# include <linux/binder.h>
> +#endif
> +
> +#include "xlat/binder_driver_commands.h"
> +#include "xlat/binder_driver_returns.h"
> +
> +static int
> +decode_binder_returns(struct tcb *tcp, struct binder_write_read *wr)
> +{
> +	if (wr->read_consumed < sizeof(uint32_t)) {
> +		tprints("[]");
> +		return 0;
> +	}
> +
> +	if (abbrev(tcp)) {
> +		tprints("[...]");
> +		return 0;
> +	}
> +
> +	char *buffer = malloc(wr->read_consumed);

At this point strace will try to malloc up to UINT_MAX bytes.
If read_consumed really comes from the kernel, this is probably
not a big deal yet, but if it doesn't, this is going to be a DoS.

> +	if (!buffer)
> +		return 1;
> +
> +	if (umoven(tcp, wr->read_buffer, wr->read_consumed, buffer)) {
> +		free(buffer);
> +		return 1;
> +	}
> +
> +	size_t pos = 0;
> +	uint32_t type;
> +	tprints("[");
> +
> +	goto print_one_read_buffer;
> +	while (pos + sizeof(uint32_t) <= wr->read_consumed) {
> +		tprints(", ");
> +
> +print_one_read_buffer:
> +		type = *(uint32_t *)(buffer + pos);
> +		if (_IOC_SIZE(type) > 0) {
> +			tprints("[");

Is it really an array?  For me it looks rather like a structure.

> +			printxval(binder_driver_returns, type, "BR_???");
> +			tprints(", ");
> +			print_quoted_string(buffer + pos + sizeof(type),
> +					_IOC_SIZE(type), 0);

At this point strace will try to access memory at address
buffer + pos + sizeof(type) + _IOC_SIZE(type) - 1

If the data comes from the kernel, this address is likely within
[buffer; buffer + wr->read_consumed)
limits, but what's going to happen if it doesn't?

> +			tprints("]");
> +		} else
> +			printxval(binder_driver_returns, type, "BR_???");
> +		pos += sizeof(uint32_t) + _IOC_SIZE(type);
> +	}
> +
> +	tprints("]");
> +	free(buffer);
> +	return 0;
> +}
> +
> +static int
> +decode_binder_commands(struct tcb *tcp, struct binder_write_read *wr)
> +{
> +	if (wr->write_size < sizeof(uint32_t)) {
> +		tprints("[]");
> +		return 0;
> +	}
> +
> +	if (abbrev(tcp)) {
> +		tprints("[...]");
> +		return 0;
> +	}
> +
> +	char *buffer = malloc(wr->write_size);

Similar to the case of read_consumed, but here write_size definitely comes
from untrusted input.

> +	if (!buffer)
> +		return 1;
> +
> +	if (umoven(tcp, wr->write_buffer, wr->write_size, buffer)) {
> +		free(buffer);
> +		return 1;
> +	}
> +
> +	size_t pos = wr->write_consumed;
> +	uint32_t type;
> +	tprints("[");
> +
> +	goto print_one_write_buffer;
> +	while (pos + sizeof(uint32_t) <= wr->write_size) {
> +		tprints(", ");
> +
> +print_one_write_buffer:
> +		type = *(uint32_t *)(buffer + pos);
> +		if (_IOC_SIZE(type) > 0) {
> +			tprints("[");
> +			printxval(binder_driver_commands, type, "BC_???");
> +			tprints(", ");
> +			print_quoted_string(buffer + pos + sizeof(type),
> +					_IOC_SIZE(type), 0);

Here it goes.  strace will try to read memory from address
buffer + pos + sizeof(type) + _IOC_SIZE(type) - 1
which may easily go outside [buffer; buffer + write_size) bounds
since both write_size and _IOC_SIZE(type) are under user control.

I might be a good idea to write a test demonstrating this crash.

> +			tprints("]");
> +		} else
> +			printxval(binder_driver_commands, type, "BC_???");
> +		pos += sizeof(uint32_t) + _IOC_SIZE(type);
> +	}
> +
> +	tprints("]");
> +	free(buffer);
> +	return 0;
> +}

BTW, as decode_binder_commands and decode_binder_returns look similar to
each other, wouldn't it be better to merge them into a single function?

> +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(tcp, &wr))
> +			return RVAL_DECODED | 1;

This will result to unbalanced curly brackets when decode_binder_commands
returns true.

> +		tprintf(", read_size=%" PRIu64 ", read_consumed=%" PRIu64 "}",
> +				(uint64_t)wr.read_size,
> +				(uint64_t)wr.read_consumed);
> +		return 0;
> +	}
> +
> +	if (syserror(tcp))
> +		return RVAL_DECODED | 1;
> +
> +	if (umove(tcp, addr, &wr))
> +		return RVAL_DECODED | 1;

These two checks could be merged into a single check.

> +	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 (decode_binder_returns(tcp, &wr))
> +		return RVAL_DECODED | 1;

This also can result to unbalanced curly brackets.

> +	tprints("}");
> +	return RVAL_DECODED | 1;
> +}
> +
> +int
> +decode_binder_version(struct tcb *tcp, long addr)
> +{
> +	struct binder_version version;
> +
> +	tprints(", ");
> +	if (umove_or_printaddr(tcp, addr, &version))
> +		return RVAL_DECODED | 1;
> +
> +	tprintf("{protocol_version=%" PRId32 "}", version.protocol_version);
> +	return RVAL_DECODED | 1;
> +}
> +
> +int
> +binder_ioctl(struct tcb *tcp, const unsigned int code, long arg)
> +{
> +	if (!verbose(tcp))
> +		return RVAL_DECODED;
> +
> +	if (code == BINDER_WRITE_READ)
> +		return decode_binder_write_read(tcp, arg);
> +
> +	if (entering(tcp)) {
> +		switch (code) {
> +		case BINDER_SET_IDLE_TIMEOUT:
> +			tprints(", ");
> +			printnum_int64(tcp, arg, "%lld");
> +			return RVAL_DECODED | 1;
> +		case BINDER_SET_MAX_THREADS:
> +			tprints(", ");
> +			printnum_int(tcp, arg, "%u");
> +			return RVAL_DECODED | 1;
> +		case BINDER_SET_IDLE_PRIORITY:
> +		case BINDER_SET_CONTEXT_MGR:
> +		case BINDER_THREAD_EXIT:
> +			tprints(", ");
> +			printnum_int(tcp, arg, "%d");
> +			return RVAL_DECODED | 1;
> +		default:
> +			break;
> +		}
> +	} else {
> +		if (code == BINDER_VERSION)
> +			return decode_binder_version(tcp, arg);
> +	}
> +
> +	return 0;
> +}
> +
> +#endif /* !(HAVE_LINUX_ANDROID_BINDER_H || __ANDROID__) */
> diff --git a/configure.ac b/configure.ac
> index 7dfa1d1..535b835 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -430,6 +430,8 @@ AC_CHECK_HEADERS([linux/bpf.h], [
>  	fi
>  ])
>  
> +AC_CHECK_HEADERS([linux/android/binder.h],,, [#include <sys/types.h>])
> +
>  AC_CHECK_TYPES([struct statfs], [
>  	AC_CHECK_MEMBERS([struct statfs.f_frsize],,, [#include <linux/types.h>
>  #include <asm/statfs.h>])
> diff --git a/defs.h b/defs.h
> index b2a7f4d..92c6e55 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -663,6 +663,7 @@ extern void print_struct_statfs64(struct tcb *tcp, long, unsigned long);
>  extern int file_ioctl(struct tcb *, const unsigned int, long);
>  extern int fs_x_ioctl(struct tcb *, const unsigned int, long);
>  extern int hdio_ioctl(struct tcb *, const unsigned int, long);
> +extern int binder_ioctl(struct tcb *, const unsigned int, long);
>  extern int loop_ioctl(struct tcb *, const unsigned int, long);
>  extern int mtd_ioctl(struct tcb *, const unsigned int, long);
>  extern int ptp_ioctl(struct tcb *, const unsigned int, long);

Please keep this list sorted.

> diff --git a/ioctl.c b/ioctl.c
> index e4b20d9..54aa9a7 100644
> --- a/ioctl.c
> +++ b/ioctl.c
> @@ -282,6 +282,10 @@ ioctl_decode(struct tcb *tcp)
>  	case 0x94:
>  		return btrfs_ioctl(tcp, code, arg);
>  #endif
> +#if defined(HAVE_LINUX_ANDROID_BINDER_H) || defined(__ANDROID__)
> +	case 'b':
> +		return binder_ioctl(tcp, code, arg);
> +#endif
>  	default:
>  		break;
>  	}
> diff --git a/xlat/binder_driver_commands.in b/xlat/binder_driver_commands.in
> new file mode 100644
> index 0000000..9652ae3
> --- /dev/null
> +++ b/xlat/binder_driver_commands.in
> @@ -0,0 +1,21 @@
> +/*
> + * These values are defined as an enum in linux/android/binder.h and must be
> + * defined here because xlat is unable to fetch them.
> + */

There is a way to use enums in xlat files, it requires some AC_CHECK_DECLS
code in configure.ac, see e.g. BTRFS_COMPRESS_* checks.


-- 
ldv