Re: [RFC 1/3] abi_spec: basic definitions of constraints, args and syscalls

From: alexander . levin
Date: Wed Jan 04 2017 - 00:05:45 EST

Next message: Masami Hiramatsu: "Re: [PATCH tip/master v3] kprobes: extable: Identify kprobes' insn-slots as kernel text area"
Previous message: Milo Kim: "[PATCH v4 1/3] ARM: dts: imx6: Add Savageboard common file"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Dec 14, 2016 at 08:46:25PM +0100, Dmitry Vyukov wrote:
> Here is my current prototype:
> https://github.com/dvyukov/linux/commit/6200a9333e78bef393f8ead41205813b94d340f3
>
> For now it can trace arguments of 4 system calls:
>
> [ 4.055483] [pid 1258] open(&00007ffdefc023a0=[], 0x0, 0x1b6)
> [ 4.055991] [pid 1258] open(&00007ffdefc023a0=[], 0x0, 0x1b6) = 3
> [ 4.056486] [pid 1258] read(0x3, &00007ffdefc01320=[], 0x1000)
> [ 4.056977] [pid 1258] read(0x3, &00007ffdefc01320=[], 0x1000) = 1780
> [ 4.057485] [pid 1258] read(0x3, &00007f316a732000=[], 0x1000)
> [ 4.057991] [pid 1258] read(0x3, &00007f316a732000=[], 0x1000) = 0
> [ 4.058488] [pid 1258] close(0x0) = 0
> [ 4.058848] [pid 1258] write(0x1, &00007f316a732000=[], 0x5)
> [ 4.059304] [pid 1258] write(0x1, &00007f316a732000=[], 0x5) = 5
> [ 4.059905] [pid 1234] close(0x0) = 0
> [ 4.060239] [pid 1234] close(0x0) = 0
>
>
> Main outstanding problems:
> - understanding length of arrays and buffers
> - understanding discriminators of unions and syscall variations

Happy new year! I've been away for a bit myself, but now back working on this.

Attached a patch on top of your commit.

There are two things (very messy, I just want to go through the concept):

- Reading the values into a generic fields struct, based on your suggestion.
There's no actual struct there, just the values - we can figure out how it'll
look like exactly, but something along this path makes sense?

tglx also raised a point that we want to read from userspace only once for
performance; it's a bit early to address performance at this stage, but it's
another advantage to pursuing this approach.

- Array/string length. Since we read all values, we can point to the array's
length by using an offset from the currect arg. So for example, in read(), the
length of the buffer is at +1 offset from the buffer. This seems to be the case
for most syscalls.

The exception here is strings which we can just define as offset == 0.

With the patch, the trace is now able to work with strings:

[ 1.234156] [pid 891] open(&00007fa7b35d4035=[ /etc/ld.so.cache ], 0x80000, 0x1)
[ 1.235244] [pid 891] open(&00007fa7b35d4035=[ /etc/ld.so.cache ], 0x80000, 0x1) = -2
[ 1.236101] [pid 891] open(&00007ffe57ca2a70=[ /lib/x86_64-linux-gnu/tls/x86_64/libc.so.6 ], 0x80000, 0xb37db168)
[ 1.237361] [pid 891] open(&00007ffe57ca2a70=[ /lib/x86_64-linux-gnu/tls/x86_64/libc.so.6 ], 0x80000, 0xb37db168) = -2
[ 1.238545] [pid 891] open(&00007ffe57ca2a70=[ /lib/x86_64-linux-gnu/tls/libc.so.6 ], 0x80000, 0xb37db168)
[ 1.239600] [pid 891] open(&00007ffe57ca2a70=[ /lib/x86_64-linux-gnu/tls/libc.so.6 ], 0x80000, 0xb37db168) = -2
[ 1.241033] [pid 891] open(&00007ffe57ca2a70=[ /lib/x86_64-linux-gnu/x86_64/libc.so.6 ], 0x80000, 0xb37db168)
[ 1.242163] [pid 891] open(&00007ffe57ca2a70=[ /lib/x86_64-linux-gnu/x86_64/libc.so.6 ], 0x80000, 0xb37db168) = -2
[ 1.243329] [pid 891] open(&00007ffe57ca2a70=[ /lib/x86_64-linux-gnu/libc.so.6 ], 0x80000, 0xb37db168)
[ 1.244712] [pid 891] open(&00007ffe57ca2a70=[ /lib/x86_64-linux-gnu/libc.so.6 ], 0x80000, 0xb37db168) = 3
[ 1.245633] [pid 891] read(0x3, &00007ffe57ca2c98=[ (null) ], 0x340)
[ 1.246334] [pid 891] read(0x3, &00007ffe57ca2c98=[ (null) ], 0x340) = 832

Does the idea makes sense?

--

Thanks,
Sasha
diff --git a/include/uapi/linux/abi_spec.h b/include/uapi/linux/abi_spec.h
index dd8ddc3..21b31f9 100644
--- a/include/uapi/linux/abi_spec.h
+++ b/include/uapi/linux/abi_spec.h
@@ -80,6 +80,7 @@ struct type {
// KIND_ARRAY
struct {
struct type *type;
+ u8 length_off;
} array;

// KIND_STRUCT
@@ -110,4 +111,15 @@ struct syscall_spec {
struct argument args[ABI_MAX_ARGS];
};

+struct syscall_values {
+ union {
+ u8 u8;
+ u16 u16;
+ u32 u32;
+ u64 u64;
+ void *ptr;
+ char *str;
+ };
+};
+
#endif
diff --git a/kernel/abi_spec.c b/kernel/abi_spec.c
index fa249bd..dbafb98 100644
--- a/kernel/abi_spec.c
+++ b/kernel/abi_spec.c
@@ -7,10 +7,11 @@
#include <linux/errno.h>
#include <linux/fcntl.h>
#include <linux/stat.h>
+#include <linux/vmalloc.h>

-typedef void (*cb_t)(void *ctx, struct type *t, int flags, const void __user *p, int post);
+typedef void (*cb_t)(void *ctx, struct type *t, int flags, const void __user *p, struct syscall_values *val, int post);

-static size_t handle_type(cb_t cb, void *ctx, struct type *t, int flags, const void __user *p);
+static size_t handle_type(cb_t cb, void *ctx, struct type *t, int flags, const void __user *p, struct syscall_values *val);

static u64 read_val(struct type *t, int flags, const void __user *p)
{
@@ -60,27 +61,34 @@ static u64 read_val(struct type *t, int flags, const void __user *p)
}
}

-static size_t handle_type(cb_t cb, void *ctx, struct type *t, int flags, const void __user *p)
+static size_t handle_type(cb_t cb, void *ctx, struct type *t, int flags, const void __user *p, struct syscall_values *val)
{
size_t off;

- cb(ctx, t, flags, p, 0);
+ cb(ctx, t, flags, p, val, 0);
switch (t->kind) {
case KIND_SCALAR: {
off = t->scalar.size;
+ val->u64 = read_val(t, flags, p);
break;
}
case KIND_PTR: {
const void __user* p1;

p1 = (const void __user*)read_val(t, flags, p);
- handle_type(cb, ctx, t->ptr.type, 0, p1);
+ handle_type(cb, ctx, t->ptr.type, 0, p1, val);
off = sizeof(void *);
break;
}
case KIND_ARRAY: {
- // TODO: don't know the size...
- // off = handle_array(cb, ctx, t, p);
+ // length_off == 0 => string
+ if (t->array.length_off == 0) {
+ u8 len = strnlen_user(p, 0x10000);
+ val->ptr = vmalloc(len);
+ if (WARN_ON(!val->ptr))
+ break;
+ strncpy_from_user(val->ptr, p, len);
+ } else {} // todo
break;
}
case KIND_STRUCT: {
@@ -95,8 +103,8 @@ static size_t handle_type(cb_t cb, void *ctx, struct type *t, int flags, const v
if (!f)
break;
if (i)
- cb(ctx, NULL, 0, NULL, 0);
- off += handle_type(cb, ctx, f, arg->flags, p + off);
+ cb(ctx, NULL, 0, NULL, val, 0);
+ off += handle_type(cb, ctx, f, arg->flags, p + off, val);
}
break;
}
@@ -109,13 +117,13 @@ static size_t handle_type(cb_t cb, void *ctx, struct type *t, int flags, const v
struct type *t1;

for (t1 = t; t1->kind == KIND_RESOURCE; t1 = t1->res.type) {}
- off = handle_type(cb, ctx, t1, flags, p);
+ off = handle_type(cb, ctx, t1, flags, p, val);
break;
}
default:
BUG();
}
- cb(ctx, t, flags, p, 1);
+ cb(ctx, t, flags, p, val, 1);
return off;
}

@@ -125,6 +133,7 @@ static void handle_syscall(cb_t cb, void *ctx, struct syscall_spec *s, va_list *
struct argument *arg;
struct type *f;
long v;
+ struct syscall_values vals[ABI_MAX_ARGS] = { 0 };

for (i = 0; i < ABI_MAX_ARGS; i++) {
arg = &s->args[i];
@@ -132,9 +141,9 @@ static void handle_syscall(cb_t cb, void *ctx, struct syscall_spec *s, va_list *
if (!f)
break;
if (i)
- cb(ctx, NULL, 0, NULL, 0);
+ cb(ctx, NULL, 0, NULL, &vals[i], 0);
v = va_arg(*ap, long);
- handle_type(cb, ctx, f, arg->flags, &v);
+ handle_type(cb, ctx, f, arg->flags, &v, &vals[i]);
}
}

@@ -148,7 +157,7 @@ static void check_retval(struct syscall_spec *s, long retval)
if (s->errno[i] == -retval)
return;
}
- __WARN_printf("syscall %s returned unexpected error %ld",
+ WARN("syscall %s returned unexpected error %ld",
s->name, retval);
}

@@ -169,7 +178,7 @@ void check_pre_printf(struct check_pre_ctx *ctx, const char *fmt, ...)
va_end(args);
}

-void check_pre_cb(void *ctx, struct type *t, int flags, const void __user *p, int post)
+void check_pre_cb(void *ctx, struct type *t, int flags, const void __user *p, struct syscall_values *val, int post)
{
if (!t) {
check_pre_printf(ctx, ", ");
@@ -188,7 +197,7 @@ void check_pre_cb(void *ctx, struct type *t, int flags, const void __user *p, in
check_pre_printf(ctx, "&%p=", (void*)read_val(t, flags, p));
break;
case KIND_ARRAY:
- check_pre_printf(ctx, post ? "]" : "[");
+ check_pre_printf(ctx, post ? " %s ]" : "[", val->ptr);
break;
case KIND_STRUCT:
check_pre_printf(ctx, post ? "}" : "{");
@@ -287,9 +296,16 @@ static struct type type_iptr = {
.scalar.size = sizeof(void *),
};

+static struct type type_string_i8 = {
+ .kind = KIND_ARRAY,
+ .array.type = &type_i8,
+ .array.length_off = 0,
+};
+
static struct type type_array_i8 = {
.kind = KIND_ARRAY,
.array.type = &type_i8,
+ .array.length_off = 1,
};

static struct type type_ptr_array_i8 = {
@@ -300,7 +316,7 @@ static struct type type_ptr_array_i8 = {
static struct type type_pathname = {
.kind = KIND_RESOURCE,
.res.res = RES_PATHNAME,
- .res.type = &type_array_i8,
+ .res.type = &type_string_i8,
};

static struct type type_ptr_pathname = {
@@ -752,5 +768,15 @@ struct syscall_spec syscall_spec_sync_file_range2 = { .name = "sync_file_range2"
struct syscall_spec syscall_spec_statfs64 = { .name = "statfs64" };
struct syscall_spec syscall_spec_fstatfs64 = { .name = "fstatfs64" };
struct syscall_spec syscall_spec_bdflush = { .name = "bdflush" };
+struct syscall_spec syscall_spec_sigaction = { .name = "sigaction" };
+struct syscall_spec syscall_spec_old_mmap = { .name = "old_mmap" };
+struct syscall_spec syscall_spec_truncate64 = { .name = "truncate64" };
+struct syscall_spec syscall_spec_ftruncate64 = { .name = "ftruncate64" };
+struct syscall_spec syscall_spec_stat64 = { .name = "stat64" };
+struct syscall_spec syscall_spec_lstat64 = { .name = "lstat64" };
+struct syscall_spec syscall_spec_fstat64 = { .name = "fstat64" };
+struct syscall_spec syscall_spec_fstatat64 = { .name = "fstatat64" };
+struct syscall_spec syscall_spec_fcntl64 = { .name = "fcntl64" };
+struct syscall_spec syscall_spec_old_select = { .name = "old_select" };

#undef $

Next message: Masami Hiramatsu: "Re: [PATCH tip/master v3] kprobes: extable: Identify kprobes' insn-slots as kernel text area"
Previous message: Milo Kim: "[PATCH v4 1/3] ARM: dts: imx6: Add Savageboard common file"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]