[PATCH] Re: your dm patch for strace

[Dmitry V. Levin]

On Thu, Aug 25, 2016 at 08:27:21AM -0400, Mikulas Patocka wrote:
> On Wed, 24 Aug 2016, Masatake YAMATO wrote:
> 
> > >> Are you talking about https://sourceforge.net/p/strace/mailman/message/34370586/ ?
> > > 
> > > Yes.
> > > 
> > >> The thread has apparently died without any follow-up from your side.
> > > 
> > > I didn't receive that last message, it was probably sent only to the 
> > > mailing list address, not to my address.
> > 
> > Can I expect you will submit updated version of the patch?
> > 
> > Masatake YAMATO
> 
> Here I'm sending the updated device mapper strace patch with the comments 
> addressed. (please send replies to my email address, I'm not on the strace 
> mailing list)
> 
> Mikulas
> 
> Index: strace/configure.ac
> ===================================================================
> --- strace.orig/configure.ac
> +++ strace/configure.ac
> @@ -363,6 +363,7 @@ AC_CHECK_HEADERS(m4_normalize([
>  	elf.h
>  	inttypes.h
>  	linux/bsg.h
> +	linux/dm-ioctl.h
>  	linux/falloc.h
>  	linux/fiemap.h
>  	linux/filter.h
> Index: strace/dm.c
> ===================================================================
> --- /dev/null
> +++ strace/dm.c
> @@ -0,0 +1,351 @@
> +#include "defs.h"
> +
> +#ifdef HAVE_LINUX_DM_IOCTL_H
> +
> +#include <sys/ioctl.h>
> +#include <linux/dm-ioctl.h>
> +
> +static void
> +dm_decode_device(const unsigned int code, const struct dm_ioctl *ioc)
> +{
> +	switch (code) {
> +	case DM_REMOVE_ALL:
> +	case DM_LIST_DEVICES:
> +	case DM_LIST_VERSIONS:
> +		break;
> +	default:
> +		if (ioc->dev)
> +			tprintf(", dev=makedev(%u, %u)",
> +				major(ioc->dev), minor(ioc->dev));
> +		if (ioc->name[0]) {
> +			tprints(", name=");
> +			print_quoted_string(ioc->name, DM_NAME_LEN,
> +					    QUOTE_0_TERMINATED);
> +		}
> +		if (ioc->uuid[0]) {
> +			tprints(", uuid=");
> +			print_quoted_string(ioc->uuid, DM_UUID_LEN,
> +					    QUOTE_0_TERMINATED);
> +		}
> +		break;
> +	}
> +}
> +
> +static void
> +dm_decode_values(struct tcb *tcp, const unsigned int code,
> +		 const struct dm_ioctl *ioc)
> +{
> +	if (entering(tcp)) {
> +		switch (code) {
> +		case DM_TABLE_LOAD:
> +			tprintf(", target_count=%"PRIu32"",
> +				ioc->target_count);
> +			break;
> +		case DM_DEV_SUSPEND:
> +			if (ioc->flags & DM_SUSPEND_FLAG)
> +				break;
> +		case DM_DEV_RENAME:
> +		case DM_DEV_REMOVE:
> +		case DM_DEV_WAIT:
> +			tprintf(", event_nr=%"PRIu32"",
> +				ioc->event_nr);
> +			break;
> +		}
> +	} else if (!syserror(tcp)) {
> +		switch (code) {
> +		case DM_DEV_CREATE:
> +		case DM_DEV_RENAME:
> +		case DM_DEV_SUSPEND:
> +		case DM_DEV_STATUS:
> +		case DM_DEV_WAIT:
> +		case DM_TABLE_LOAD:
> +		case DM_TABLE_CLEAR:
> +		case DM_TABLE_DEPS:
> +		case DM_TABLE_STATUS:
> +		case DM_TARGET_MSG:
> +			tprintf(", target_count=%"PRIu32"",
> +				ioc->target_count);
> +			tprintf(", open_count=%"PRIu32"",
> +				ioc->open_count);
> +			tprintf(", event_nr=%"PRIu32"",
> +				ioc->event_nr);
> +			break;
> +		}
> +	}
> +}
> +
> +#include "xlat/dm_flags.h"
> +
> +static void
> +dm_decode_flags(const struct dm_ioctl *ioc)
> +{
> +	tprints(", flags=");
> +	printflags(dm_flags, ioc->flags, "DM_???");
> +}
> +
> +static void
> +dm_decode_dm_target_spec(struct tcb *tcp, const struct dm_ioctl *ioc,
> +			 const char *extra, uint32_t extra_size)
> +{
> +	uint32_t i;
> +	uint32_t offset = ioc->data_start;
> +	for (i = 0; i < ioc->target_count; i++) {
> +		if (offset + (uint32_t)sizeof(struct dm_target_spec) >= offset &&
> +		    offset + (uint32_t)sizeof(struct dm_target_spec) < extra_size) {
> +			const struct dm_target_spec *s =
> +				(const struct dm_target_spec *)(extra + offset);
> +			tprintf(", {sector_start=%"PRIu64", length=%"PRIu64"",
> +				(uint64_t)s->sector_start, (uint64_t)s->length);
> +			if (!entering(tcp))
> +				tprintf(", status=%"PRId32"", s->status);
> +			tprints(", target_type=");
> +			print_quoted_string(s->target_type, DM_MAX_TYPE_NAME,
> +					    QUOTE_0_TERMINATED);
> +			tprints(", string=");
> +			print_quoted_string((const char *)(s + 1), extra_size -
> +					    (offset +
> +					    sizeof(struct dm_target_spec)),
> +					    QUOTE_0_TERMINATED);
> +			tprintf("}");
> +			if (entering(tcp))
> +				offset = offset + s->next;
> +			else
> +				offset = ioc->data_start + s->next;

This code trusts s->next; unfortunately, strace cannot trust syscall
arguments, otherwise anything may happen, e.g.

$ cat ioctl_dm.c
#include <sys/ioctl.h>
#include <linux/dm-ioctl.h>
int main(void)
{
	struct {
		struct dm_ioctl ioc;
		struct dm_target_spec spec;
		int i;
	} s = {
		.spec = { 0 },
		.ioc = {
			.version[0] = DM_VERSION_MAJOR,
			.data_size = sizeof(s),
			.data_start = sizeof(s.ioc),
			.target_count = -1U
		}
	};
	return !ioctl(-1, DM_TABLE_LOAD, &s);
}
$ strace -veioctl ./ioctl_dm

btw, this parser lacks tests.  How one can easily verify that it works
correctly?  For example how a test for strace ioctl parser may look like
see tests/ioctl_* files.


-- 
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/20160912/69748f24/attachment.bin>