[lttng-dev] [PATCH lttng-modules] Add support for i2c tracepoints

[mathieu.desnoyers@efficios.com (Mathieu Desnoyers)]

----- 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 <simon.marchi@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