[PATCH 3/3] Add btrfs ioctl support.

[Dmitry V. Levin]

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
-------------- 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/20160401/82037d7f/attachment.bin>