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
pgpXmKwpD5xwA.pgp
Description: PGP signature
------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e
_______________________________________________ Strace-devel mailing list Strace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/strace-devel