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

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

----- 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 <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

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com