From mboxrd@z Thu Jan 1 00:00:00 1970 From: mathieu.desnoyers@efficios.com (Mathieu Desnoyers) Date: Tue, 4 Oct 2016 19:19:38 +0000 (UTC) Subject: [lttng-dev] [PATCH lttng-modules] Add support for i2c tracepoints In-Reply-To: <1386914197.45790.1475602673694.JavaMail.zimbra@efficios.com> References: <20161003182115.30428-1-simon.marchi@ericsson.com> <93496556.44682.1475533300165.JavaMail.zimbra@efficios.com> <1386914197.45790.1475602673694.JavaMail.zimbra@efficios.com> Message-ID: <1613721840.45882.1475608778928.JavaMail.zimbra@efficios.com> ----- On Oct 4, 2016, at 1:37 PM, Mathieu Desnoyers mathieu.desnoyers at efficios.com wrote: > ----- On Oct 4, 2016, at 1:08 PM, Simon Marchi simon.marchi at ericsson.com wrote: > >> On 16-10-03 06:21 PM, Mathieu Desnoyers wrote: >>> ----- On Oct 3, 2016, at 2:21 PM, Simon Marchi simon.marchi at ericsson.com wrote: >>> >>>> This patch teaches lttng-modules about the i2c tracepoints in the Linux >>>> kernel. >>>> >>>> It contains the following tracepoints: >>>> >>>> * i2c_write >>>> * i2c_read >>>> * i2c_reply >>>> * i2c_result >>>> >>>> I translated the fields and assignments from the kernel's >>>> include/trace/events/i2c.h as well as I could. I also tried building >>>> this module against a kernel without CONFIG_I2C, and it built fine (the >>>> required types are unconditionally defined). So I don't think any "#if >>>> CONFIG_I2C" or similar are required. >>> >>> Hi Simon, >>> >>> The global approach taken in this patch looks good. However, it raises the >>> question of sensitive information. I'm pretty sure i2c buffers may contain >>> keyboard keystrokes, which is sensitive info (passwords). It's the same >>> with read/write system call payloads, network application-level payload, >>> and HID keystrokes. Back in the days of lttng 0.x, I made sure it would >>> not allow users to gather a trace with sensitive information by mistake. >>> You really had to clearly enable gathering of such sensitive information. >>> >>> One possibility in lttng-modules 2.x would be to introduce a module >>> parameter for each lttng-modules probe that would specify whether >>> sensitive information should be traced. I could be specified by the >>> system administrator at module load time. The default would be that >>> no sensitive information is gathered. >>> >>> I think we should *not* make this a per-session/dynamic option, because >>> it's really something that the machine administrator should decide on >>> a machine-by-machine basis. In production environments, this would be >>> expected to disallow gathering sensitive info (default). >>> >>> In specific use-cases where security is no concern (e.g. embedded board >>> during development), this could be explicitly enabled by the developer >>> with root access by passing the module flag at load time. >>> >>> What do you think ? >>> >>> Thanks, >>> >>> Mathieu >> >> It makes sense. I just tested it and it works fine, here's a prototype. >> >> On a development machine, it's easy to enable once and for all by putting >> >> options lttng-probe-i2c trace_sensitive_data=1 >> >> in /etc/modprobe.d/lttng.conf for example. >> >> Here I have put the knob in the i2c module itself, which means that if we have >> this situation in other modules, we'll have to copy the code that defines the >> parameter. That gives us a module-level granularity, which can be an upside >> (more control over which modules collect sensitive data) or a downside (more >> complex, need to pass the option too all modules). >> >> I assume it would be possible to put the parameter in the lttng-tracer module, >> on which all other modules depend, in order to have an all-or-nothing >> approach. >> >> One cool thing about module parameters is that they can be modified dynamically >> by writing to sysfs >> (/sys/module/lttng_probe_i2c/parameters/trace_sensitive_data). >> So you can change the setting mid-trace, I tried it and it works. >> >> If the gathering of sensitive info is off, the sequence of bytes is just empty. >> Since you have the length of the message as a different field, the length of the >> sequence does not match the former. The trace viewer can probably use this >> mismatch as a sign that the data was omitted, and that it might not have been an >> actual empty message. > > Perhaps we could rename the option to "trace_sensitive_payload" ? Or: allow_sensitive_payload_extraction ? Thanks, Mathieu > > I think we should keep it at the probe level, like you do in your prototype. > >> >> >> From 69aa1c3d73b5b0dbf031b90c6e38d4b2a0ad8699 Mon Sep 17 00:00:00 2001 >> From: Simon Marchi >> Date: Tue, 4 Oct 2016 11:38:19 -0400 >> Subject: [PATCH] Provide knob to control tracing of i2c buffer contents >> >> --- >> instrumentation/events/lttng-module/i2c.h | 6 ++++-- >> probes/lttng-probe-i2c.c | 7 +++++++ >> 2 files changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/instrumentation/events/lttng-module/i2c.h >> b/instrumentation/events/lttng-module/i2c.h >> index 68ce80d..1ede276 100644 >> --- a/instrumentation/events/lttng-module/i2c.h >> +++ b/instrumentation/events/lttng-module/i2c.h >> @@ -26,7 +26,8 @@ LTTNG_TRACEPOINT_EVENT(i2c_write, >> ctf_integer(__u16, addr, msg->addr) >> ctf_integer(__u16, flags, msg->flags) >> ctf_integer(__u16, len, msg->len) >> - ctf_sequence_hex(__u8, buf, msg->buf, __u16, msg->len) >> + ctf_sequence_hex(__u8, buf, trace_sensitive_data ? msg->buf : NULL, >> + __u16, trace_sensitive_data ? msg->len : 0) > > Since "trace_sensitive_data" can be changed at runtime concurrently > with tracing, we should load it with LOAD_ONCE(), keep it in a local > variable (TRACEPOINT_EVENT_CODE, TP_locvar and TP_code), and use that > in as argument. Otherwise the compiler can perform two fetch of that > variable, and its value may change in between, thus leading to a > NULL pointer dereference (OOPS). > >> ) >> ) >> >> @@ -63,7 +64,8 @@ LTTNG_TRACEPOINT_EVENT(i2c_reply, >> ctf_integer(__u16, addr, msg->addr) >> ctf_integer(__u16, flags, msg->flags) >> ctf_integer(__u16, len, msg->len) >> - ctf_sequence_hex(__u8, buf, msg->buf, __u16, msg->len) >> + ctf_sequence_hex(__u8, buf, trace_sensitive_data ? msg->buf : NULL, >> + __u16, trace_sensitive_data ? msg->len : 0) >> ) >> ) >> > > Same here. > > Can you also fold it with your i2c patch ? > > Thanks, > > Mathieu > > >> diff --git a/probes/lttng-probe-i2c.c b/probes/lttng-probe-i2c.c >> index 94ebdc0..fd30a73 100644 >> --- a/probes/lttng-probe-i2c.c >> +++ b/probes/lttng-probe-i2c.c >> @@ -21,6 +21,7 @@ >> */ >> >> #include >> +#include >> #include >> >> /* >> @@ -36,6 +37,12 @@ >> #define CREATE_TRACE_POINTS >> #define TRACE_INCLUDE_PATH instrumentation/events/lttng-module >> >> +static int trace_sensitive_data = 0; >> +module_param(trace_sensitive_data, int, 0644); >> +MODULE_PARM_DESC(trace_sensitive_data, "Whether to trace possibly sensitive " >> + "data (i2c buffer contents) or not (1 or " >> + "0, default: 0)."); >> + >> #include >> >> MODULE_LICENSE("GPL and additional rights"); >> -- >> 2.10.1 > > -- > Mathieu Desnoyers > EfficiOS Inc. > http://www.efficios.com -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com