----- On Oct 4, 2016, at 1:08 PM, Simon Marchi simon.mar...@ericsson.com wrote:
> On 16-10-03 06:21 PM, Mathieu Desnoyers wrote: >> ----- On Oct 3, 2016, at 2:21 PM, Simon Marchi simon.mar...@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 <simon.mar...@ericsson.com> > 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 <linux/module.h> > +#include <linux/moduleparam.h> > #include <lttng-tracer.h> > > /* > @@ -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 <instrumentation/events/lttng-module/i2c.h> > > MODULE_LICENSE("GPL and additional rights"); > -- > 2.10.1 -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com _______________________________________________ lttng-dev mailing list lttng-dev@lists.lttng.org https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev