From mboxrd@z Thu Jan 1 00:00:00 1970 From: mathieu.desnoyers@efficios.com (Mathieu Desnoyers) Date: Tue, 4 Oct 2016 17:37:53 +0000 (UTC) Subject: [lttng-dev] [PATCH lttng-modules] Add support for i2c tracepoints In-Reply-To: References: <20161003182115.30428-1-simon.marchi@ericsson.com> <93496556.44682.1475533300165.JavaMail.zimbra@efficios.com> Message-ID: <1386914197.45790.1475602673694.JavaMail.zimbra@efficios.com> ----- 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" ? 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