| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAM_iQpUJ5b89HZ6EcYx3n4xSy-uqrvVu4n7G=8ddEd102fHpoQ@mail.gmail.com>
Date: Mon, 18 Nov 2019 20:59:34 -0800
From: Cong Wang <xiyou.wangcong@...il.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH v3] tracing: Introduce trace event injection
On Tue, Nov 12, 2019 at 8:31 AM Steven Rostedt <rostedt@...dmis.org> wrote:
>
>
> Again, sorry for the late review. Things have just been crazy :-/
I know, the only problem is even I myself almost forget how the
code works, I have to spend some time to warm up. :-(
> > +config TRACE_EVENT_INJECT
> > + bool "Trace event injection"
> > + depends on TRACING
> > + default n
>
> No need for the above "default n", that's the default.
Yeah, I know you want to save one line, let's do it!
> > +static int
> > +parse_field(char *str, struct trace_event_call *call,
> > + struct ftrace_event_field **pf, u64 *pv)
> > +{
> > + struct ftrace_event_field *field;
> > + char *field_name;
> > + int s, i = 0;
> > + char *p;
> > + int len;
> > + u64 val;
> > +
> > + if (!str[i])
> > + return 0;
> > + /* First find the field to associate to */
> > + while (isspace(str[i]))
> > + i++;
> > + s = i;
> > + while (isalnum(str[i]) || str[i] == '_')
> > + i++;
> > + len = i - s;
> > + if (!len)
> > + return -EINVAL;
> > +
> > + field_name = kmemdup_nul(str + s, len, GFP_KERNEL);
> > + if (!field_name)
> > + return -ENOMEM;
> > + field = trace_find_event_field(call, field_name);
> > + kfree(field_name);
> > + if (!field)
> > + return -ENOENT;
> > +
> > + *pf = field;
> > + p = strchr(str + i, '=');
>
> Hmm, the above here will parse out:
>
> "field some garbarge = one"
>
> It will skip the some garbage, and jump straight to 1.
Good catch! Let's write a test case for it and fix it accordingly.
>
> > + if (!p)
> > + return -EINVAL;
> > + i = p + 1 - str;
> > + while (isspace(str[i]))
> > + i++;
> > + s = i;
> > + if (isdigit(str[i]) || str[i] == '-') {
> > + char *num, c;
> > + int ret;
> > +
> > + /* Make sure the field is not a string */
> > + if (is_string_field(field))
> > + return -EINVAL;
> > +
> > + if (str[i] == '-')
> > + i++;
> > +
> > + /* We allow 0xDEADBEEF */
> > + while (isalnum(str[i]))
> > + i++;
> > + num = str + s;
> > + c = str[i];
>
> Probably should verify that c is whitespace or '\0'.
Hmm, I think we should be more strict on errors. Let's me
write a test case for this too.
>
> > + str[i] = '\0';
> > + /* Make sure it is a value */
> > + if (field->is_signed)
> > + ret = kstrtoll(num, 0, &val);
> > + else
> > + ret = kstrtoull(num, 0, &val);
> > + str[i] = c;
> > + if (ret)
> > + return ret;
> > +
> > + *pv = val;
> > + return i;
> > + } else if (str[i] == '\'' || str[i] == '"') {
> > + char q = str[i];
> > +
> > + /* Make sure the field is OK for strings */
> > + if (!is_string_field(field))
> > + return -EINVAL;
> > +
> > + for (i++; str[i]; i++) {
> > + if (str[i] == '\\' && str[i + 1]) {
> > + i++;
> > + continue;
> > + }
> > + if (str[i] == q)
> > + break;
> > + }
> > + if (!str[i])
> > + return -EINVAL;
> > +
> > + /* Skip quotes */
> > + s++;
> > + len = i - s;
> > + if (len >= MAX_FILTER_STR_VAL)
> > + return -EINVAL;
> > +
> > + *pv = (unsigned long)(str + s);
> > + str[i] = 0;
> > + /* go past the last quote */
> > + i++;
> > + return i;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int trace_get_entry_size(struct trace_event_call *call)
> > +{
> > + struct ftrace_event_field *field;
> > + struct list_head *head;
> > + int size = 0;
> > +
> > + head = trace_get_fields(call);
> > + list_for_each_entry(field, head, link) {
> > + if (field->size + field->offset > size)
> > + size = field->size + field->offset;
> > + }
> > +
> > + return size;
> > +}
> > +
> > +static void *trace_alloc_entry(struct trace_event_call *call, int *size)
> > +{
> > + int entry_size = trace_get_entry_size(call);
> > + struct ftrace_event_field *field;
> > + struct list_head *head;
> > + void *entry = NULL;
> > +
> > + /* We need an extra '\0' at the end. */
> > + entry = kzalloc(entry_size + 1, GFP_KERNEL);
> > + if (!entry)
> > + return NULL;
> > +
> > + head = trace_get_fields(call);
> > + list_for_each_entry(field, head, link) {
> > + if (!is_string_field(field))
> > + continue;
> > + if (field->filter_type == FILTER_STATIC_STRING)
> > + continue;
> > + if (field->filter_type == FILTER_DYN_STRING) {
> > + u32 *str_item;
> > + int str_len = 0;
> > + int str_loc = entry_size & 0xffff;
> > +
> > + str_item = (u32 *)(entry + field->offset);
> > + *str_item = (str_len << 16) | str_loc;
>
> str_len is always zero here, right? Might as well just make this
>
> *str_item = str_loc;
>
Hmm, I don't even remember whether I forgot to set str_len to some value
or simply we can just remove it...
Sorry for it, you know this code was written more than 6 months ago...
>
> > + } else {
> > + char **paddr;
> > +
> > + paddr = (char **)(entry + field->offset);
> > + *paddr = "";
>
> Hmm, I know this is a default pointer to a sting, but perhaps we should
> make it point to NULL, and not a string defined by this function?
Again, I don't remember why but I guess it might be intentionally
reserved to empty string.
>
> > + }
> > + }
> > +
> > + *size = entry_size + 1;
> > + return entry;
> > +}
> > +
> > +#define INJECT_STRING "STATIC STRING CAN NOT BE INJECTED"
> > +
> > +/* Caller is responsible to free the *pentry. */
> > +static int parse_entry(char *str, struct trace_event_call *call, void **pentry)
> > +{
> > + struct ftrace_event_field *field;
> > + unsigned long irq_flags;
> > + void *entry = NULL;
> > + int entry_size;
> > + u64 val;
> > + int len;
> > +
> > + entry = trace_alloc_entry(call, &entry_size);
> > + *pentry = entry;
> > + if (!entry)
> > + return -ENOMEM;
> > +
> > + local_save_flags(irq_flags);
> > + tracing_generic_entry_update(entry, call->event.type, irq_flags,
> > + preempt_count());
> > +
> > + while ((len = parse_field(str, call, &field, &val)) > 0) {
> > + if (is_function_field(field))
>
> Why not allow injecting function fields?
I don't quite remember, but I think it is probably due to I don't have
such a use case for testing rasdaemon. Do you have any use case
on your mind?
>
> > + return -EINVAL;
> > +
> > + if (is_string_field(field)) {
> > + char *addr = (char *)(unsigned long) val;
> > +
> > + if (field->filter_type == FILTER_STATIC_STRING) {
> > + strlcpy(entry + field->offset, addr, field->size);
> > + } else if (field->filter_type == FILTER_DYN_STRING) {
> > + int str_len = strlen(addr) + 1;
> > + int str_loc = entry_size & 0xffff;
> > + u32 *str_item;
> > +
> > + entry_size += str_len;
> > + *pentry = krealloc(entry, entry_size, GFP_KERNEL);
> > + entry = *pentry;
> > + if (!entry)
> > + return -ENOMEM;
>
> The above leaks memory, as if *pentry is NULL then the old entry is not
> freed. The above needs to be:
>
> *pentry = krealloc(entry, entry_size, GFP_KERNEL);
> if (!*pentry) {
> kfree(entry);
> return -ENOMEM;
> }
> entry = *pentry;
Good catch!
>
> > +
> > + strlcpy(entry + (entry_size - str_len), addr, str_len);
> > + str_item = (u32 *)(entry + field->offset);
> > + *str_item = (str_len << 16) | str_loc;
> > + } else {
> > + char **paddr;
> > +
> > + /* TODO: can we find the constant string? */
>
> probably not ;-)
Ok, I thought you want to keep it, then let's remove it.
Thanks!
Powered by blists - more mailing lists