Re: [PATCH lttng-modules] Add support for i2c tracepoints

Re: [PATCH lttng-modules] Add support for i2c tracepoints

[Mathieu Desnoyers]

----- On Oct 3, 2016, at 2:21 PM, Simon Marchi simon.marchi@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

> 
> Signed-off-by: Simon Marchi <simon.marchi@ericsson.com>
> ---
> instrumentation/events/lttng-module/i2c.h | 89 +++++++++++++++++++++++++++++++
> probes/Kbuild                             |  1 +
> probes/lttng-probe-i2c.c                  | 47 ++++++++++++++++
> 3 files changed, 137 insertions(+)
> create mode 100644 instrumentation/events/lttng-module/i2c.h
> create mode 100644 probes/lttng-probe-i2c.c
> 
> diff --git a/instrumentation/events/lttng-module/i2c.h
> b/instrumentation/events/lttng-module/i2c.h
> new file mode 100644
> index 0000000..68ce80d
> --- /dev/null
> +++ b/instrumentation/events/lttng-module/i2c.h
> @@ -0,0 +1,89 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM i2c
> +
> +#if !defined(LTTNG_TRACE_I2C_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define LTTNG_TRACE_I2C_H
> +
> +#include <probes/lttng-tracepoint-event.h>
> +
> +#ifndef _TRACE_I2C_DEF_
> +#define _TRACE_I2C_DEF_
> +
> +#endif /* _TRACE_I2C_DEF_ */
> +
> +/*
> + * __i2c_transfer() write request
> + */
> +LTTNG_TRACEPOINT_EVENT(i2c_write,
> +
> +	TP_PROTO(const struct i2c_adapter *adap, const struct i2c_msg *msg, int num),
> +
> +	TP_ARGS(adap, msg, num),
> +
> +	TP_FIELDS(
> +		ctf_integer(int, adapter_nr, adap->nr)
> +		ctf_integer(__u16, msg_nr, num)
> +		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)
> +	)
> +)
> +
> +/*
> + * __i2c_transfer() read request
> + */
> +LTTNG_TRACEPOINT_EVENT(i2c_read,
> +
> +	TP_PROTO(const struct i2c_adapter *adap, const struct i2c_msg *msg, int num),
> +
> +	TP_ARGS(adap, msg, num),
> +
> +	TP_FIELDS(
> +		ctf_integer(int, adapter_nr, adap->nr)
> +		ctf_integer(__u16, msg_nr, num)
> +		ctf_integer(__u16, addr, msg->addr)
> +		ctf_integer(__u16, flags, msg->flags)
> +		ctf_integer(__u16, len, msg->len)
> +	)
> +)
> +
> +/*
> + * __i2c_transfer() read reply
> + */
> +LTTNG_TRACEPOINT_EVENT(i2c_reply,
> +
> +	TP_PROTO(const struct i2c_adapter *adap, const struct i2c_msg *msg, int num),
> +
> +	TP_ARGS(adap, msg, num),
> +
> +	TP_FIELDS(
> +		ctf_integer(int, adapter_nr, adap->nr)
> +		ctf_integer(__u16, msg_nr, num)
> +		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)
> +	)
> +)
> +
> +/*
> + * __i2c_transfer() result
> + */
> +LTTNG_TRACEPOINT_EVENT(i2c_result,
> +
> +	TP_PROTO(const struct i2c_adapter *adap, int num, int ret),
> +
> +	TP_ARGS(adap, num, ret),
> +
> +	TP_FIELDS(
> +		ctf_integer(int, adapter_nr, adap->nr)
> +		ctf_integer(__u16, nr_msgs, num)
> +		ctf_integer(__s16, ret, ret)
> +	)
> +)
> +
> +#endif /*  LTTNG_TRACE_I2C_H */
> +
> +/* This part must be outside protection */
> +#include <probes/define_trace.h>
> diff --git a/probes/Kbuild b/probes/Kbuild
> index 8ae9a6b..ca7c0b1 100644
> --- a/probes/Kbuild
> +++ b/probes/Kbuild
> @@ -5,6 +5,7 @@ include $(TOP_LTTNG_MODULES_DIR)/Makefile.ABI.workarounds
> ccflags-y += -I$(TOP_LTTNG_MODULES_DIR)
> 
> obj-$(CONFIG_LTTNG) += lttng-probe-sched.o
> +obj-$(CONFIG_LTTNG) += lttng-probe-i2c.o
> obj-$(CONFIG_LTTNG) += lttng-probe-irq.o
> obj-$(CONFIG_LTTNG) += lttng-probe-timer.o
> obj-$(CONFIG_LTTNG) += lttng-probe-kmem.o
> diff --git a/probes/lttng-probe-i2c.c b/probes/lttng-probe-i2c.c
> new file mode 100644
> index 0000000..94ebdc0
> --- /dev/null
> +++ b/probes/lttng-probe-i2c.c
> @@ -0,0 +1,47 @@
> +/*
> + * probes/lttng-probe-i2c.c
> + *
> + * LTTng i2c probes.
> + *
> + * Copyright (C) 2012 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; only
> + * version 2.1 of the License.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include <linux/module.h>
> +#include <lttng-tracer.h>
> +
> +/*
> + * Create the tracepoint static inlines from the kernel to validate that our
> + * trace event macros match the kernel we run on.
> + */
> +#include <trace/events/i2c.h>
> +
> +/*
> + * Create LTTng tracepoint probes.
> + */
> +#define LTTNG_PACKAGE_BUILD
> +#define CREATE_TRACE_POINTS
> +#define TRACE_INCLUDE_PATH instrumentation/events/lttng-module
> +
> +#include <instrumentation/events/lttng-module/i2c.h>
> +
> +MODULE_LICENSE("GPL and additional rights");
> +MODULE_AUTHOR("Mathieu Desnoyers <mathieu.desnoyers@efficios.com>");
> +MODULE_DESCRIPTION("LTTng i2c probes");
> +MODULE_VERSION(__stringify(LTTNG_MODULES_MAJOR_VERSION) "."
> +	__stringify(LTTNG_MODULES_MINOR_VERSION) "."
> +	__stringify(LTTNG_MODULES_PATCHLEVEL_VERSION)
> +	LTTNG_MODULES_EXTRAVERSION);
> --
> 2.9.3
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

Re: [PATCH lttng-modules] Add support for i2c tracepoints

[Simon Marchi]

On 16-10-03 06:21 PM, Mathieu Desnoyers wrote:
> ----- On Oct 3, 2016, at 2:21 PM, Simon Marchi simon.marchi@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.


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

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

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



_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH lttng-modules] Add support for i2c tracepoints

[Mathieu Desnoyers]

----- On Oct 4, 2016, at 1:08 PM, Simon Marchi simon.marchi@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@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
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH lttng-modules] Add support for i2c tracepoints

[Mathieu Desnoyers]

----- On Oct 4, 2016, at 1:37 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

> ----- On Oct 4, 2016, at 1:08 PM, Simon Marchi simon.marchi@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@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
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH lttng-modules] Add support for i2c tracepoints

[Simon Marchi]

On 16-10-04 01:37 PM, Mathieu Desnoyers wrote:
> Perhaps we could rename the option to "trace_sensitive_payload" ?

Of all those:

- trace_sensitive_data
- trace_sensitive_payload
- allow_sensitive_payload_extraction
- extract_sensitive_payload

I prefer "extract_sensitive_payload".  But it's your call, just tell me which
one you prefer.

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

Ahh good catch.  I'll look up those things you mentioned, but do you
have a usage pattern in mind that we could use here?  Where would the
local variable be?  I know of gcc's statement expression, we could use
that, but do you have something else in mind?

>> 	)
>> )
>>
>> @@ -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 ?

I'll send a v2 of the original patch once it's clear how to do the load once thingy.

Thanks!

Simon

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH lttng-modules] Add support for i2c tracepoints

[Mathieu Desnoyers]

----- On Oct 4, 2016, at 3:48 PM, Simon Marchi simon.marchi@ericsson.com wrote:

> On 16-10-04 01:37 PM, Mathieu Desnoyers wrote:
>> Perhaps we could rename the option to "trace_sensitive_payload" ?
> 
> Of all those:
> 
> - trace_sensitive_data
> - trace_sensitive_payload
> - allow_sensitive_payload_extraction
> - extract_sensitive_payload
> 
> I prefer "extract_sensitive_payload".  But it's your call, just tell me which
> one you prefer.

Let's go for "extract_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).
> 
> Ahh good catch.  I'll look up those things you mentioned, but do you
> have a usage pattern in mind that we could use here?  Where would the
> local variable be?  I know of gcc's statement expression, we could use
> that, but do you have something else in mind?

See http://lttng.org/docs/#doc-lttng-tracepoint-event-code

Note that there are slight changes to TP_code() in 2.8. See
source for details.


> 
>>> 	)
>>> )
>>>
>>> @@ -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 ?
> 
> I'll send a v2 of the original patch once it's clear how to do the load once
> thingy.

Allright, thanks!

Mathieu

> 
> Thanks!
> 
> Simon

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

[PATCH lttng-modules] Add support for i2c tracepoints

[Simon Marchi]

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.

Signed-off-by: Simon Marchi <simon.marchi@ericsson.com>
---
 instrumentation/events/lttng-module/i2c.h | 89 +++++++++++++++++++++++++++++++
 probes/Kbuild                             |  1 +
 probes/lttng-probe-i2c.c                  | 47 ++++++++++++++++
 3 files changed, 137 insertions(+)
 create mode 100644 instrumentation/events/lttng-module/i2c.h
 create mode 100644 probes/lttng-probe-i2c.c

diff --git a/instrumentation/events/lttng-module/i2c.h b/instrumentation/events/lttng-module/i2c.h
new file mode 100644
index 0000000..68ce80d
--- /dev/null
+++ b/instrumentation/events/lttng-module/i2c.h
@@ -0,0 +1,89 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM i2c
+
+#if !defined(LTTNG_TRACE_I2C_H) || defined(TRACE_HEADER_MULTI_READ)
+#define LTTNG_TRACE_I2C_H
+
+#include <probes/lttng-tracepoint-event.h>
+
+#ifndef _TRACE_I2C_DEF_
+#define _TRACE_I2C_DEF_
+
+#endif /* _TRACE_I2C_DEF_ */
+
+/*
+ * __i2c_transfer() write request
+ */
+LTTNG_TRACEPOINT_EVENT(i2c_write,
+
+	TP_PROTO(const struct i2c_adapter *adap, const struct i2c_msg *msg, int num),
+
+	TP_ARGS(adap, msg, num),
+
+	TP_FIELDS(
+		ctf_integer(int, adapter_nr, adap->nr)
+		ctf_integer(__u16, msg_nr, num)
+		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)
+	)
+)
+
+/*
+ * __i2c_transfer() read request
+ */
+LTTNG_TRACEPOINT_EVENT(i2c_read,
+
+	TP_PROTO(const struct i2c_adapter *adap, const struct i2c_msg *msg, int num),
+
+	TP_ARGS(adap, msg, num),
+
+	TP_FIELDS(
+		ctf_integer(int, adapter_nr, adap->nr)
+		ctf_integer(__u16, msg_nr, num)
+		ctf_integer(__u16, addr, msg->addr)
+		ctf_integer(__u16, flags, msg->flags)
+		ctf_integer(__u16, len, msg->len)
+	)
+)
+
+/*
+ * __i2c_transfer() read reply
+ */
+LTTNG_TRACEPOINT_EVENT(i2c_reply,
+
+	TP_PROTO(const struct i2c_adapter *adap, const struct i2c_msg *msg, int num),
+
+	TP_ARGS(adap, msg, num),
+
+	TP_FIELDS(
+		ctf_integer(int, adapter_nr, adap->nr)
+		ctf_integer(__u16, msg_nr, num)
+		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)
+	)
+)
+
+/*
+ * __i2c_transfer() result
+ */
+LTTNG_TRACEPOINT_EVENT(i2c_result,
+
+	TP_PROTO(const struct i2c_adapter *adap, int num, int ret),
+
+	TP_ARGS(adap, num, ret),
+
+	TP_FIELDS(
+		ctf_integer(int, adapter_nr, adap->nr)
+		ctf_integer(__u16, nr_msgs, num)
+		ctf_integer(__s16, ret, ret)
+	)
+)
+
+#endif /*  LTTNG_TRACE_I2C_H */
+
+/* This part must be outside protection */
+#include <probes/define_trace.h>
diff --git a/probes/Kbuild b/probes/Kbuild
index 8ae9a6b..ca7c0b1 100644
--- a/probes/Kbuild
+++ b/probes/Kbuild
@@ -5,6 +5,7 @@ include $(TOP_LTTNG_MODULES_DIR)/Makefile.ABI.workarounds
 ccflags-y += -I$(TOP_LTTNG_MODULES_DIR)
 
 obj-$(CONFIG_LTTNG) += lttng-probe-sched.o
+obj-$(CONFIG_LTTNG) += lttng-probe-i2c.o
 obj-$(CONFIG_LTTNG) += lttng-probe-irq.o
 obj-$(CONFIG_LTTNG) += lttng-probe-timer.o
 obj-$(CONFIG_LTTNG) += lttng-probe-kmem.o
diff --git a/probes/lttng-probe-i2c.c b/probes/lttng-probe-i2c.c
new file mode 100644
index 0000000..94ebdc0
--- /dev/null
+++ b/probes/lttng-probe-i2c.c
@@ -0,0 +1,47 @@
+/*
+ * probes/lttng-probe-i2c.c
+ *
+ * LTTng i2c probes.
+ *
+ * Copyright (C) 2012 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; only
+ * version 2.1 of the License.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include <linux/module.h>
+#include <lttng-tracer.h>
+
+/*
+ * Create the tracepoint static inlines from the kernel to validate that our
+ * trace event macros match the kernel we run on.
+ */
+#include <trace/events/i2c.h>
+
+/*
+ * Create LTTng tracepoint probes.
+ */
+#define LTTNG_PACKAGE_BUILD
+#define CREATE_TRACE_POINTS
+#define TRACE_INCLUDE_PATH instrumentation/events/lttng-module
+
+#include <instrumentation/events/lttng-module/i2c.h>
+
+MODULE_LICENSE("GPL and additional rights");
+MODULE_AUTHOR("Mathieu Desnoyers <mathieu.desnoyers@efficios.com>");
+MODULE_DESCRIPTION("LTTng i2c probes");
+MODULE_VERSION(__stringify(LTTNG_MODULES_MAJOR_VERSION) "."
+	__stringify(LTTNG_MODULES_MINOR_VERSION) "."
+	__stringify(LTTNG_MODULES_PATCHLEVEL_VERSION)
+	LTTNG_MODULES_EXTRAVERSION);
-- 
2.9.3

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev