Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE

[Mathieu Desnoyers]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@xxxxxxxxxxx>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@xxxxxxxxxxxx>
> Cc: "Ingo Molnar" <mingo@xxxxxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx, "Ingo Molnar" <mingo@xxxxxxxxxx>, "Thomas
> Gleixner" <tglx@xxxxxxxxxxxxx>, "Rusty Russell" <rusty@xxxxxxxxxxxxxxx>, "David Howells" <dhowells@xxxxxxxxxx>,
> "Greg Kroah-Hartman" <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Thursday, February 13, 2014 3:45:07 PM
> Subject: Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
> 
> On Thu, 13 Feb 2014 15:41:30 +0000 (UTC)
> Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx> wrote:
> 
> 
> > Yes, exactly, presuming that by "supporting" you mean CONFIG_MODULE_SIG=y.
> > Loading an unsigned module then taints the kernel, and taints the module
> > with TAINT_FORCED_MODULE even though "modprobe --force" was never used.
> 
> OK, this IS a major bug and needs to be fixed. This explains a couple
> of reports I received about tracepoints not working, and I never
> figured out why. Basically, they even did this:
> 
> 
> 	trace_printk("before tracepoint\n");
> 	trace_some_trace_point();
> 	trace_printk("after tracepoint\n");
> 
> Enabled the tracepoint (it shows up as enabled and working in the
> tools, but not the trace), but in the trace they would get:
> 
> 	before tracepoint
> 	after tracepoint
> 
> and never get the actual tracepoint. But as they were debugging
> something else, it was just thought that this was their bug. But it
> baffled me to why that tracepoint wasn't working even though nothing in
> the dmesg had any errors about tracepoints.
> 
> Well, this now explains it. If you compile a kernel with the following
> options:
> 
> CONFIG_MODULE_SIG=y
> # CONFIG_MODULE_SIG_FORCE is not set
> # CONFIG_MODULE_SIG_ALL is not set
> 
> You now just disabled (silently) all tracepoints in modules. WITH NO
> FREAKING ERROR MESSAGE!!!
> 
> The tracepoints will show up in /sys/kernel/debug/tracing/events, they
> will show up in perf list, you can enable them in either perf or the
> debugfs, but they will never actually be executed. You will just get
> silence even though everything appeared to be working just fine.
> 
> I tested this out. I was not able to get any tracepoints in modules
> working with the above config options.
> 
> There's two bugs here. One, the lack of MODULE_SIG_ALL, will
> make all modules non signed during the build process. That means that
> all modules when loaded will be tainted as FORCED. As Mathieu stated,
> you do not need the --force flag here, it just needs to see that the
> kernel supports signatures but the module is not signed. In this case,
> it just calls add_taint_module(), tainting the module with
> FORCED_MODULE. You get a message like this:
> 
>  sunrpc: module verification failed: signature and/or required key missing -
>  tainting kernel
> 
> 
> Now when this module adds its tracepoint, since it is marked with a
> FORCE_MODULE taint flag, the tracepoint code just ignores it and does
> not do any processing at all.
> 
> Worse yet, the tracepoint code never gives any feedback if a tracepoint
> was enabled or not. That is, if a tracepoint was never initialized when
> the module was loaded, the tracepoint will still report success at
> time of enabling, that it was registered (and tracing) even though it is
> not.
> 
> At the end of this email, I added a patch that fixes that. But I have to
> concur that Mathieu did find a bug. There is no reason to disable
> tracepoints in modules if someone simply has the above configs enabled
> (and disabled)!
> 
> Below is the patch that warns if the tracepoint is not enabled for
> whatever reason.
> 
> Signed-off-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
> 
> -- Steve
> 
> 
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 29f2654..b85f68f 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -62,6 +62,7 @@ struct tracepoint_entry {
>  	struct hlist_node hlist;
>  	struct tracepoint_func *funcs;
>  	int refcount;	/* Number of times armed. 0 if disarmed. */
> +	int enabled;	/* Tracepoint enabled */
>  	char name[0];
>  };
>  
> @@ -237,6 +238,7 @@ static struct tracepoint_entry *add_tracepoint(const char
> *name)
>  	memcpy(&e->name[0], name, name_len);
>  	e->funcs = NULL;
>  	e->refcount = 0;
> +	e->enabled = 0;
>  	hlist_add_head(&e->hlist, head);
>  	return e;
>  }
> @@ -316,6 +318,7 @@ static void tracepoint_update_probe_range(struct
> tracepoint * const *begin,
>  		if (mark_entry) {
>  			set_tracepoint(&mark_entry, *iter,
>  					!!mark_entry->refcount);
> +			mark_entry->enabled = !!mark_entry->refcount;
>  		} else {
>  			disable_tracepoint(*iter);
>  		}
> @@ -380,6 +383,8 @@ tracepoint_add_probe(const char *name, void *probe, void
> *data)
>  int tracepoint_probe_register(const char *name, void *probe, void *data)
>  {
>  	struct tracepoint_func *old;
> +	struct tracepoint_entry *entry;
> +	int ret = 0;
>  
>  	mutex_lock(&tracepoints_mutex);
>  	old = tracepoint_add_probe(name, probe, data);
> @@ -388,9 +393,15 @@ int tracepoint_probe_register(const char *name, void
> *probe, void *data)
>  		return PTR_ERR(old);
>  	}
>  	tracepoint_update_probes();		/* may update entry */
> +	entry = get_tracepoint(name);
> +	/* Make sure the entry was enabled */
> +	if (!entry || !entry->enabled) {
> +		tracepoint_entry_remove_probe(entry, probe, data);

I'm OK with returning some kind of feedback about whether the tracepoint is
enabled or not, but there is one use-case I care about this change breaks:
registering a tracepoint probe for a module that is not yet loaded. The change
you propose here removes the probe if no tracepoint is found when the probe is
registered.

Another way to do this, without requiring changes to the existing
tracepoint_probe_register() API, is to simply add e.g. (better function
names welcome):

int tracepoint_has_callsites(const char *name)
{
    struct tracepoint_entry *entry;
    int ret = 0;

    mutex_lock(&tracepoints_mutex);
    entry = get_tracepoint(name);
    if (entry && entry->refcount) {
        ret = 1;
    }
    mutex_unlock(&tracepoints_mutex);
    return ret;
}

So tools which care about providing feedback to their users about the
fact that tracepoint callsites are not there can call this new function.
The advantage is that it would not change the return values of the existing
API. Also, it would be weird to have tracepoint_probe_register() return
an error value but leave the tracepoint in a registered state. Moving this
into a separate query function seems more consistent IMHO.

Thoughts ?

Thanks,

Mathieu


> +		ret = -ENODEV;
> +	}
>  	mutex_unlock(&tracepoints_mutex);
>  	release_probes(old);
> -	return 0;
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(tracepoint_probe_register);
>  
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/