On Thu, Mar 31, 2016 at 12:34:18AM -0400, Jeff Mahoney wrote:
> + case BTRFS_IOC_DEV_REPLACE: { /* RW */
> + struct btrfs_ioctl_dev_replace_args args;
> +
> + if (entering(tcp))
> + tprints(", ");
> +
> + if (umove_or_printaddr(tcp, arg, &args))
> + break;
> +
> + if (entering(tcp)) {
> + tprints("{cmd=");
> + printxvals(args.cmd, "BTRFS_IOCTL_DEV_REPLACE_CMD_???",
> + btrfs_dev_replace_cmds, NULL);
> + if (args.cmd == BTRFS_IOCTL_DEV_REPLACE_CMD_START) {
> + tprintf(", start={srcdevid=%" PRI__u64 ", "
> + "cont_reading_from_srcdev_mode=%" PRI__u64
> + ", srcdev_name=\"%s\", tgtdev_name=\"%s\"}",
Please do not print user input this way, it can contain arbitrary data,
not necessarily NUL-terminated. It's easy to reproduce on entering syscall.
There is a function called print_quoted_string() that does the right thing
in cases like this, it also does proper quoting, see e.g. block.c
There are more cases in btrfs.c where user input is printed
using %s format, please check and correct them.
> + args.start.srcdevid,
> + args.start.cont_reading_from_srcdev_mode,
> + args.start.srcdev_name,
> + args.start.tgtdev_name);
> + }
> + tprints("}");
> + return 0;
> + }
> +
> + tprints(" => {result=");
In most of RW cases (e.g. BTRFS_IOC_DEV_INFO) you did it right,
but here you can get something like this:
BTRFS_IOC_DEV_REPLACE, {cmd=...}0xdeadbeef
instead of expected
BTRFS_IOC_DEV_REPLACE, {cmd=...} => 0xdeadbeef
> + printxvals(args.result, "BTRFS_IOCTL_DEV_REPLACE_RESULT_???",
> + btrfs_dev_replace_results, NULL);
> + if (args.cmd == BTRFS_IOCTL_DEV_REPLACE_CMD_STATUS) {
> + char buf[10];
sizeof("HH:MM:SS")?
> + time_t time;
> + tprints(", ");
> + printxvals(args.status.replace_state,
> + "BTRFS_IOCTL_DEV_REPLACE_STATE_???",
> + btrfs_dev_replace_state, NULL);
> + snprintf(buf, sizeof(buf), ", %" PRI__u64
What this comma is doing here?
> + ".%" PRI__u64 "%%",
> + args.status.progress_1000 / 10,
> + args.status.progress_1000 % 10);
> + tprintf(", progress_1000=[%" PRI__u64 "(%s)], ",
> + args.status.progress_1000, buf);
Let's print percentage inline, without temporary buffer.
> + case BTRFS_IOC_INO_PATHS: { /* RW */
> + struct btrfs_ioctl_ino_path_args args;
> + struct btrfs_data_container fspath;
> + uint64_t i;
> +
> + if (entering(tcp))
> + tprints(", ");
> + else if (syserror(tcp))
> + break;
> + else
> + tprints(" => ");
> +
> + if (umove_or_printaddr(tcp, arg, &args))
> + break;
> +
> + tprints("{");
> +
> + if (entering(tcp)) {
> + tprintf("inum=%" PRI__u64 ", size=%" PRI__u64 "}",
> + args.inum, args.size);
> + return 0;
> + }
> +
> + if (umoven_or_printaddr(tcp, args.fspath, args.size, &fspath))
> + break;
This might lead to "=> {0xdeadbeef".
> + case BTRFS_IOC_LOGICAL_INO: { /* RW */
> + struct btrfs_ioctl_logical_ino_args args;
> + struct btrfs_data_container *inodes;
> + uint64_t i;
> +
> + if (entering(tcp))
> + tprints(", ");
> + else if (syserror(tcp))
> + break;
> + else
> + tprints(" => ");
> +
> + if (umove_or_printaddr(tcp, arg, &args))
> + break;
> +
> + tprints("{");
> +
> + if (entering(tcp)) {
> + tprintf("logical=%" PRI__u64 ", size=%" PRI__u64 "}",
> + args.logical, args.size);
> + return 0;
> + }
> +
> + tprints("inodes=");
> + inodes = malloc(args.size);
I fell somewhat uneasy with malloc of arbitrary 64-bit size even
if it has been returned by the kernel.
> + if (!inodes) {
> + tprintf("%#lx", (unsigned long)args.inodes);
> + break;
> + }
> +
> + if (umoven_or_printaddr(tcp, args.inodes, args.size, inodes)) {
> + free(inodes);
> + break;
> + }
> +
> + tprintf("{bytes_left=%u, bytes_missing=%u, "
> + "elem_cnt=%u, elem_missed=%u, val=[",
> + inodes->bytes_left, inodes->bytes_missing,
> + inodes->elem_cnt, inodes->elem_missed);
> + for (i = 0; i < inodes->elem_cnt; i += 3) {
elem_cnt has type __u32; do you really want to print the whole array? It
might be a good idea to clamp printed length to something like max_strlen
at least in abbrev mode.
> + if (args.progress == -1ULL && abbrev(tcp))
> + tprints("-1");
> + else
> + btrfs_print_objectid(args.progress);
Not sure why here and in other places you prefer printing -1ULL
not as "-1" in non-abbrev mode.
> + size = sizeof(*info) * args.dest_count;
> + info = malloc(size);
> + if (!info) {
> + tprintf("%#lx}", arg + sizeof(args));
> + break;
> + }
> +
> + if (umoven_or_printaddr(tcp, arg + sizeof(args), size, info)) {
> + tprints("}");
> + free(info);
> + break;
> + }
> +
> + if (entering(tcp)) {
> + tprints("[");
> + for (i = 0; i < args.dest_count; i++) {
> + if (i)
> + tprints(", ");
> + tprintf("{fd=%" PRI__s64 ", "
> + "logical_offset=%" PRI__u64 "}",
> + info[i].fd, info[i].logical_offset);
> + }
> + tprints("]");
> + return 0;
free(info)?
> + case BTRFS_IOC_SEND: { /* W */
> + struct btrfs_ioctl_send_args args;
> + __u64 *sources;
> + size_t size;
> + uint64_t i;
That's a strange mix of __u64 and uint64_t.
> + tprints(", ");
> + if (umove_or_printaddr(tcp, arg, &args))
> + break;
> +
> + tprintf("{send_fd=%" PRI__s64 ", clone_sources_count=%" PRI__u64
> + ", clone_sources=", args.send_fd,
> + args.clone_sources_count);
> +
> + size = args.clone_sources_count * sizeof(*args.clone_sources);
As clone_sources_count has type __u64, this is a potential integer
overflow, easily reproducible on entering syscall.
> + sources = malloc(size);
malloc of arbitrary 64-bit value specified by used on entering syscall?
Please impose some sane limit on this.
> + if (!sources) {
> + tprintf("%#lx}", (unsigned long)args.clone_sources);
> + break;
> + }
> +
> + if (umoven_or_printaddr(tcp, (unsigned long)args.clone_sources,
> + size, sources)) {
> + tprintf("}");
> + free(sources);
> + break;
> + }
> +
> + tprints("[");
> + for (i = 0; i < args.clone_sources_count; i++) {
clone_sources_count has type __u64; do you really want to print the whole
array? It might be a good idea to clamp printed length to something like
max_strlen.
--
ldv
pgpKTVNTqu7P3.pgp
Description: PGP signature
------------------------------------------------------------------------------ Transform Data into Opportunity. Accelerate data analysis in your applications with Intel Data Analytics Acceleration Library. Click to learn more. http://pubads.g.doubleclick.net/gampad/clk?id=278785471&iu=/4140
_______________________________________________ Strace-devel mailing list Strace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/strace-devel
