[PATCH 1/3] Add decoding for binder ioctls
Dmitry V. Levin
ldv at altlinux.org
Mon May 30 13:33:17 UTC 2016
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
-------------- 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/20160530/3cfb356b/attachment.bin>
More information about the Strace-devel
mailing list