* Re: [PATCH lttng-modules] Add support for i2c tracepoints [not found] <20161003182115.30428-1-simon.marchi@ericsson.com> @ 2016-10-03 22:21 ` Mathieu Desnoyers [not found] ` <93496556.44682.1475533300165.JavaMail.zimbra@efficios.com> 1 sibling, 0 replies; 7+ messages in thread From: Mathieu Desnoyers @ 2016-10-03 22:21 UTC (permalink / raw) To: Simon Marchi; +Cc: lttng-dev ----- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <93496556.44682.1475533300165.JavaMail.zimbra@efficios.com>]
* Re: [PATCH lttng-modules] Add support for i2c tracepoints [not found] ` <93496556.44682.1475533300165.JavaMail.zimbra@efficios.com> @ 2016-10-04 17:08 ` Simon Marchi [not found] ` <b110360d-8be9-aa0a-408c-08c866eb96f6@ericsson.com> 1 sibling, 0 replies; 7+ messages in thread From: Simon Marchi @ 2016-10-04 17:08 UTC (permalink / raw) To: Mathieu Desnoyers; +Cc: lttng-dev 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
[parent not found: <b110360d-8be9-aa0a-408c-08c866eb96f6@ericsson.com>]
* Re: [PATCH lttng-modules] Add support for i2c tracepoints [not found] ` <b110360d-8be9-aa0a-408c-08c866eb96f6@ericsson.com> @ 2016-10-04 17:37 ` Mathieu Desnoyers [not found] ` <1386914197.45790.1475602673694.JavaMail.zimbra@efficios.com> 1 sibling, 0 replies; 7+ messages in thread From: Mathieu Desnoyers @ 2016-10-04 17:37 UTC (permalink / raw) To: Simon Marchi; +Cc: lttng-dev ----- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <1386914197.45790.1475602673694.JavaMail.zimbra@efficios.com>]
* Re: [PATCH lttng-modules] Add support for i2c tracepoints [not found] ` <1386914197.45790.1475602673694.JavaMail.zimbra@efficios.com> @ 2016-10-04 19:19 ` Mathieu Desnoyers 2016-10-04 19:48 ` Simon Marchi [not found] ` <f09f33cd-54d1-a8e9-2fbb-46a62a77ff89@ericsson.com> 2 siblings, 0 replies; 7+ messages in thread From: Mathieu Desnoyers @ 2016-10-04 19:19 UTC (permalink / raw) To: Simon Marchi; +Cc: lttng-dev ----- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH lttng-modules] Add support for i2c tracepoints [not found] ` <1386914197.45790.1475602673694.JavaMail.zimbra@efficios.com> 2016-10-04 19:19 ` Mathieu Desnoyers @ 2016-10-04 19:48 ` Simon Marchi [not found] ` <f09f33cd-54d1-a8e9-2fbb-46a62a77ff89@ericsson.com> 2 siblings, 0 replies; 7+ messages in thread From: Simon Marchi @ 2016-10-04 19:48 UTC (permalink / raw) To: Mathieu Desnoyers; +Cc: lttng-dev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <f09f33cd-54d1-a8e9-2fbb-46a62a77ff89@ericsson.com>]
* Re: [PATCH lttng-modules] Add support for i2c tracepoints [not found] ` <f09f33cd-54d1-a8e9-2fbb-46a62a77ff89@ericsson.com> @ 2016-10-04 21:21 ` Mathieu Desnoyers 0 siblings, 0 replies; 7+ messages in thread From: Mathieu Desnoyers @ 2016-10-04 21:21 UTC (permalink / raw) To: Simon Marchi; +Cc: lttng-dev ----- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH lttng-modules] Add support for i2c tracepoints @ 2016-10-03 18:21 Simon Marchi 0 siblings, 0 replies; 7+ messages in thread From: Simon Marchi @ 2016-10-03 18:21 UTC (permalink / raw) To: lttng-dev; +Cc: 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-10-04 21:21 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20161003182115.30428-1-simon.marchi@ericsson.com>
2016-10-03 22:21 ` [PATCH lttng-modules] Add support for i2c tracepoints Mathieu Desnoyers
[not found] ` <93496556.44682.1475533300165.JavaMail.zimbra@efficios.com>
2016-10-04 17:08 ` Simon Marchi
[not found] ` <b110360d-8be9-aa0a-408c-08c866eb96f6@ericsson.com>
2016-10-04 17:37 ` Mathieu Desnoyers
[not found] ` <1386914197.45790.1475602673694.JavaMail.zimbra@efficios.com>
2016-10-04 19:19 ` Mathieu Desnoyers
2016-10-04 19:48 ` Simon Marchi
[not found] ` <f09f33cd-54d1-a8e9-2fbb-46a62a77ff89@ericsson.com>
2016-10-04 21:21 ` Mathieu Desnoyers
2016-10-03 18:21 Simon Marchi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.