| Message ID | 20211216173511.10390-12-beaub@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | user_events: Enable user processes to create and write to trace events | expand |
On Thu, 16 Dec 2021 09:35:10 -0800 Beau Belgrave <beaub@linux.microsoft.com> wrote: > Add validation to ensure data is at or greater than the min size for the > fields of the event. If a dynamic array is used and is a type of char, > ensure null termination of the array exists. OK, looks good to me except a few nitpicks. Reveiewed-by: Masami Hiramatsu <mhiramat@kernel.org> I added some comments below. > > Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com> > --- > kernel/trace/trace_events_user.c | 147 +++++++++++++++++++++++++++---- > 1 file changed, 132 insertions(+), 15 deletions(-) > > diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c > index fa3e26281fc3..58b8c9607c80 100644 > --- a/kernel/trace/trace_events_user.c > +++ b/kernel/trace/trace_events_user.c > @@ -64,9 +64,11 @@ struct user_event { > struct dyn_event devent; > struct hlist_node node; > struct list_head fields; > + struct list_head validators; > atomic_t refcnt; > int index; > int flags; > + int min_size; > }; > > /* > @@ -81,8 +83,17 @@ struct user_event_refs { > struct user_event *events[]; > }; > > +#define VALIDATOR_ENSURE_NULL (1 << 0) > +#define VALIDATOR_REL (1 << 1) > + > +struct user_event_validator { > + struct list_head link; > + int offset; > + int flags; > +}; > + > typedef void (*user_event_func_t) (struct user_event *user, struct iov_iter *i, > - void *tpdata); > + void *tpdata, bool *faulted); Why don't you just return "int" value? ;-) > > static int user_event_parse(char *name, char *args, char *flags, > struct user_event **newuser); > @@ -214,6 +225,17 @@ static int user_field_size(const char *type) > return -EINVAL; > } > > +static void user_event_destroy_validators(struct user_event *user) > +{ > + struct user_event_validator *validator, *next; > + struct list_head *head = &user->validators; > + > + list_for_each_entry_safe(validator, next, head, link) { > + list_del(&validator->link); > + kfree(validator); > + } > +} > + > static void user_event_destroy_fields(struct user_event *user) > { > struct ftrace_event_field *field, *next; > @@ -229,13 +251,43 @@ static int user_event_add_field(struct user_event *user, const char *type, > const char *name, int offset, int size, > int is_signed, int filter_type) > { > + struct user_event_validator *validator; > struct ftrace_event_field *field; > + int validator_flags = 0; > > field = kmalloc(sizeof(*field), GFP_KERNEL); > > if (!field) > return -ENOMEM; > > + if (str_has_prefix(type, "__data_loc ")) > + goto add_validator; > + > + if (str_has_prefix(type, "__rel_loc ")) { > + validator_flags |= VALIDATOR_REL; > + goto add_validator; > + } > + > + goto add_field; > + > +add_validator: > + if (strstr(type, "char") != 0) > + validator_flags |= VALIDATOR_ENSURE_NULL; > + > + validator = kmalloc(sizeof(*validator), GFP_KERNEL); > + > + if (!validator) { > + kfree(field); > + return -ENOMEM; > + } > + > + validator->flags = validator_flags; > + validator->offset = offset; > + > + /* Want sequential access when validating */ > + list_add_tail(&validator->link, &user->validators); > + > +add_field: > field->type = type; > field->name = name; > field->offset = offset; > @@ -245,6 +297,12 @@ static int user_event_add_field(struct user_event *user, const char *type, > > list_add(&field->link, &user->fields); > > + /* > + * Min size from user writes that are required, this does not include > + * the size of trace_entry (common fields). > + */ > + user->min_size = (offset + size) - sizeof(struct trace_entry); > + > return 0; > } > > @@ -516,6 +574,7 @@ static int destroy_user_event(struct user_event *user) > clear_bit(user->index, page_bitmap); > hash_del(&user->node); > > + user_event_destroy_validators(user); > kfree(user->call.print_fmt); > kfree(EVENT_NAME(user)); > kfree(user); > @@ -537,15 +596,49 @@ static struct user_event *find_user_event(char *name, u32 *outkey) > return NULL; > } > > +static int user_event_validate(struct user_event *user, void *data, int len) > +{ > + struct list_head *head = &user->validators; > + struct user_event_validator *validator; > + void *pos, *end = data + len; > + u32 loc, offset, size; > + > + list_for_each_entry(validator, head, link) { > + pos = data + validator->offset; > + > + /* Already done min_size check, no bounds check here */ > + loc = *(u32 *)pos; > + offset = loc & 0xffff; > + size = loc >> 16; > + > + if (likely(validator->flags & VALIDATOR_REL)) > + pos += offset + sizeof(loc); > + else > + pos = data + offset; > + > + pos += size; > + > + if (unlikely(pos > end)) > + return -EFAULT; > + > + if (likely(validator->flags & VALIDATOR_ENSURE_NULL)) > + if (unlikely(*(char *)(pos - 1) != 0)) As we discussed in the previous version, isn't it '\0' ? (just a style comment) > + return -EFAULT; > + } > + > + return 0; > +} > + > /* > * Writes the user supplied payload out to a trace file. > */ > static void user_event_ftrace(struct user_event *user, struct iov_iter *i, > - void *tpdata) > + void *tpdata, bool *faulted) > { > struct trace_event_file *file; > struct trace_entry *entry; > struct trace_event_buffer event_buffer; > + size_t size = sizeof(*entry) + i->count; > > file = (struct trace_event_file *)tpdata; > > @@ -555,19 +648,25 @@ static void user_event_ftrace(struct user_event *user, struct iov_iter *i, > return; > > /* Allocates and fills trace_entry, + 1 of this is data payload */ > - entry = trace_event_buffer_reserve(&event_buffer, file, > - sizeof(*entry) + i->count); > + entry = trace_event_buffer_reserve(&event_buffer, file, size); > > if (unlikely(!entry)) > return; > > - if (unlikely(!copy_nofault(entry + 1, i->count, i))) { > - __trace_event_discard_commit(event_buffer.buffer, > - event_buffer.event); > - return; > - } > + if (unlikely(!copy_nofault(entry + 1, i->count, i))) > + goto discard; OK, this is a fault error. > + > + if (!list_empty(&user->validators) && > + unlikely(user_event_validate(user, entry, size))) > + goto discard; But this maybe an invalid parameter error. Thank you, > > trace_event_buffer_commit(&event_buffer); > + > + return; > +discard: > + *faulted = true; > + __trace_event_discard_commit(event_buffer.buffer, > + event_buffer.event); > } > > #ifdef CONFIG_PERF_EVENTS > @@ -622,7 +721,7 @@ static void user_event_bpf(struct user_event *user, struct iov_iter *i) > * Writes the user supplied payload out to perf ring buffer or eBPF program. > */ > static void user_event_perf(struct user_event *user, struct iov_iter *i, > - void *tpdata) > + void *tpdata, bool *faulted) > { > struct hlist_head *perf_head; > > @@ -645,14 +744,21 @@ static void user_event_perf(struct user_event *user, struct iov_iter *i, > > perf_fetch_caller_regs(regs); > > - if (unlikely(!copy_nofault(perf_entry + 1, i->count, i))) { > - perf_swevent_put_recursion_context(context); > - return; > - } > + if (unlikely(!copy_nofault(perf_entry + 1, i->count, i))) > + goto discard; > + > + if (!list_empty(&user->validators) && > + unlikely(user_event_validate(user, perf_entry, size))) > + goto discard; > > perf_trace_buf_submit(perf_entry, size, context, > user->call.event.type, 1, regs, > perf_head, NULL); > + > + return; > +discard: > + *faulted = true; > + perf_swevent_put_recursion_context(context); > } > } > #endif > @@ -967,6 +1073,7 @@ static int user_event_parse(char *name, char *args, char *flags, > > INIT_LIST_HEAD(&user->class.fields); > INIT_LIST_HEAD(&user->fields); > + INIT_LIST_HEAD(&user->validators); > > user->tracepoint.name = name; > > @@ -1015,6 +1122,7 @@ static int user_event_parse(char *name, char *args, char *flags, > return 0; > put_user: > user_event_destroy_fields(user); > + user_event_destroy_validators(user); > kfree(user); > return ret; > } > @@ -1072,6 +1180,9 @@ static ssize_t user_events_write_core(struct file *file, struct iov_iter *i) > if (unlikely(user == NULL)) > return -ENOENT; > > + if (unlikely(i->count < user->min_size)) > + return -EINVAL; > + > tp = &user->tracepoint; > > /* > @@ -1083,10 +1194,13 @@ static ssize_t user_events_write_core(struct file *file, struct iov_iter *i) > user_event_func_t probe_func; > struct iov_iter copy; > void *tpdata; > + bool faulted; > > if (unlikely(iov_iter_fault_in_readable(i, i->count))) > return -EFAULT; > > + faulted = false; > + > rcu_read_lock_sched(); > > probe_func_ptr = rcu_dereference_sched(tp->funcs); > @@ -1096,11 +1210,14 @@ static ssize_t user_events_write_core(struct file *file, struct iov_iter *i) > copy = *i; > probe_func = probe_func_ptr->func; > tpdata = probe_func_ptr->data; > - probe_func(user, ©, tpdata); > + probe_func(user, ©, tpdata, &faulted); > } while ((++probe_func_ptr)->func); > } > > rcu_read_unlock_sched(); > + > + if (unlikely(faulted)) > + return -EFAULT; > } > > return ret; > -- > 2.17.1 >
On Thu, Dec 23, 2021 at 09:08:22AM +0900, Masami Hiramatsu wrote: > On Thu, 16 Dec 2021 09:35:10 -0800 > Beau Belgrave <beaub@linux.microsoft.com> wrote: > > > Add validation to ensure data is at or greater than the min size for the > > fields of the event. If a dynamic array is used and is a type of char, > > ensure null termination of the array exists. > > OK, looks good to me except a few nitpicks. > > Reveiewed-by: Masami Hiramatsu <mhiramat@kernel.org> > > I added some comments below. > > > > > Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com> > > --- > > kernel/trace/trace_events_user.c | 147 +++++++++++++++++++++++++++---- > > 1 file changed, 132 insertions(+), 15 deletions(-) > > > > diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c > > index fa3e26281fc3..58b8c9607c80 100644 > > --- a/kernel/trace/trace_events_user.c > > +++ b/kernel/trace/trace_events_user.c > > @@ -64,9 +64,11 @@ struct user_event { > > struct dyn_event devent; > > struct hlist_node node; > > struct list_head fields; > > + struct list_head validators; > > atomic_t refcnt; > > int index; > > int flags; > > + int min_size; > > }; > > > > /* > > @@ -81,8 +83,17 @@ struct user_event_refs { > > struct user_event *events[]; > > }; > > > > +#define VALIDATOR_ENSURE_NULL (1 << 0) > > +#define VALIDATOR_REL (1 << 1) > > + > > +struct user_event_validator { > > + struct list_head link; > > + int offset; > > + int flags; > > +}; > > + > > typedef void (*user_event_func_t) (struct user_event *user, struct iov_iter *i, > > - void *tpdata); > > + void *tpdata, bool *faulted); > > Why don't you just return "int" value? ;-) > There can be more than one callback attached per-probe, and in all cases where a return value is needed is for a faulted (or would have faulted) case. This allows less branches when data is being traced/logged as the return value does not need to be checked (nor should it short circuit other probes that are attached). > > > > static int user_event_parse(char *name, char *args, char *flags, > > struct user_event **newuser); > > @@ -214,6 +225,17 @@ static int user_field_size(const char *type) > > return -EINVAL; > > } > > [..] > > +static int user_event_validate(struct user_event *user, void *data, int len) > > +{ > > + struct list_head *head = &user->validators; > > + struct user_event_validator *validator; > > + void *pos, *end = data + len; > > + u32 loc, offset, size; > > + > > + list_for_each_entry(validator, head, link) { > > + pos = data + validator->offset; > > + > > + /* Already done min_size check, no bounds check here */ > > + loc = *(u32 *)pos; > > + offset = loc & 0xffff; > > + size = loc >> 16; > > + > > + if (likely(validator->flags & VALIDATOR_REL)) > > + pos += offset + sizeof(loc); > > + else > > + pos = data + offset; > > + > > + pos += size; > > + > > + if (unlikely(pos > end)) > > + return -EFAULT; > > + > > + if (likely(validator->flags & VALIDATOR_ENSURE_NULL)) > > + if (unlikely(*(char *)(pos - 1) != 0)) > > As we discussed in the previous version, isn't it '\0' ? > (just a style comment) > Sure, there are a few dangling around that I missed. I'll fix them. > > + return -EFAULT; > > + } > > + > > + return 0; > > +} > > + > > /* > > * Writes the user supplied payload out to a trace file. > > */ > > static void user_event_ftrace(struct user_event *user, struct iov_iter *i, > > - void *tpdata) > > + void *tpdata, bool *faulted) > > { > > struct trace_event_file *file; > > struct trace_entry *entry; > > struct trace_event_buffer event_buffer; > > + size_t size = sizeof(*entry) + i->count; > > > > file = (struct trace_event_file *)tpdata; > > > > @@ -555,19 +648,25 @@ static void user_event_ftrace(struct user_event *user, struct iov_iter *i, > > return; > > > > /* Allocates and fills trace_entry, + 1 of this is data payload */ > > - entry = trace_event_buffer_reserve(&event_buffer, file, > > - sizeof(*entry) + i->count); > > + entry = trace_event_buffer_reserve(&event_buffer, file, size); > > > > if (unlikely(!entry)) > > return; > > > > - if (unlikely(!copy_nofault(entry + 1, i->count, i))) { > > - __trace_event_discard_commit(event_buffer.buffer, > > - event_buffer.event); > > - return; > > - } > > + if (unlikely(!copy_nofault(entry + 1, i->count, i))) > > + goto discard; > > OK, this is a fault error. > > > + > > + if (!list_empty(&user->validators) && > > + unlikely(user_event_validate(user, entry, size))) > > + goto discard; > > But this maybe an invalid parameter error. > Yes, but it has to be an invalid parameter that would have caused a possible fault in a worse place. In my mind, I still treat it as a fault case whether the user did it intentionally or not :) Thanks, -Beau
Hi Beau, On Mon, 3 Jan 2022 10:53:08 -0800 Beau Belgrave <beaub@linux.microsoft.com> wrote: [...] > > > typedef void (*user_event_func_t) (struct user_event *user, struct iov_iter *i, > > > - void *tpdata); > > > + void *tpdata, bool *faulted); > > > > Why don't you just return "int" value? ;-) > > > > There can be more than one callback attached per-probe, and in all cases > where a return value is needed is for a faulted (or would have faulted) > case. This allows less branches when data is being traced/logged as the > return value does not need to be checked (nor should it short circuit > other probes that are attached). Would you mean overwriting the 'faulted' ? If so, you can do something like faulted = 0; for_each_user_event_func(user_event_func) { faulted |= user_event_func(); } if (faulted) ... But I think if one user_event_func() fails to access the user data, other funcs also fail. In this case, it is faster to skip others than repeating faults. [...] > > > @@ -555,19 +648,25 @@ static void user_event_ftrace(struct user_event *user, struct iov_iter *i, > > > return; > > > > > > /* Allocates and fills trace_entry, + 1 of this is data payload */ > > > - entry = trace_event_buffer_reserve(&event_buffer, file, > > > - sizeof(*entry) + i->count); > > > + entry = trace_event_buffer_reserve(&event_buffer, file, size); > > > > > > if (unlikely(!entry)) > > > return; > > > > > > - if (unlikely(!copy_nofault(entry + 1, i->count, i))) { > > > - __trace_event_discard_commit(event_buffer.buffer, > > > - event_buffer.event); > > > - return; > > > - } > > > + if (unlikely(!copy_nofault(entry + 1, i->count, i))) > > > + goto discard; > > > > OK, this is a fault error. > > > > > + > > > + if (!list_empty(&user->validators) && > > > + unlikely(user_event_validate(user, entry, size))) > > > + goto discard; > > > > But this maybe an invalid parameter error. > > > > Yes, but it has to be an invalid parameter that would have caused a > possible fault in a worse place. In my mind, I still treat it as a fault > case whether the user did it intentionally or not :) OK, I got it. Thank you, > > Thanks, > -Beau
On Fri, Jan 07, 2022 at 08:32:52AM +0900, Masami Hiramatsu wrote: > Hi Beau, > > On Mon, 3 Jan 2022 10:53:08 -0800 > Beau Belgrave <beaub@linux.microsoft.com> wrote: > > [...] > > > > typedef void (*user_event_func_t) (struct user_event *user, struct iov_iter *i, > > > > - void *tpdata); > > > > + void *tpdata, bool *faulted); > > > > > > Why don't you just return "int" value? ;-) > > > > > > > There can be more than one callback attached per-probe, and in all cases > > where a return value is needed is for a faulted (or would have faulted) > > case. This allows less branches when data is being traced/logged as the > > return value does not need to be checked (nor should it short circuit > > other probes that are attached). > > Would you mean overwriting the 'faulted' ? If so, you can do something like > > faulted = 0; > for_each_user_event_func(user_event_func) { > faulted |= user_event_func(); > } > if (faulted) > ... > Yeah, could OR it in, I don't see a big difference though to be honest :) > But I think if one user_event_func() fails to access the user data, > other funcs also fail. In this case, it is faster to skip others than > repeating faults. eBPF will not fault when perf/ftrace could, at the very least we want to ensure that callbacks get a chance to see data even if it faulted elsewhere. This ensures that we are not blind to the fact it is happening at least when eBPF is being used. We cannot guarantee probe/callback order. This has been a problem in the past for us, we've seen data disappear later to find out it was possibly due to a page fault occurring. In later versions I would like to add internal tracepoints for these conditions (and some others) so we can further track when they occur. Thanks, -Beau
diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c index fa3e26281fc3..58b8c9607c80 100644 --- a/kernel/trace/trace_events_user.c +++ b/kernel/trace/trace_events_user.c @@ -64,9 +64,11 @@ struct user_event { struct dyn_event devent; struct hlist_node node; struct list_head fields; + struct list_head validators; atomic_t refcnt; int index; int flags; + int min_size; }; /* @@ -81,8 +83,17 @@ struct user_event_refs { struct user_event *events[]; }; +#define VALIDATOR_ENSURE_NULL (1 << 0) +#define VALIDATOR_REL (1 << 1) + +struct user_event_validator { + struct list_head link; + int offset; + int flags; +}; + typedef void (*user_event_func_t) (struct user_event *user, struct iov_iter *i, - void *tpdata); + void *tpdata, bool *faulted); static int user_event_parse(char *name, char *args, char *flags, struct user_event **newuser); @@ -214,6 +225,17 @@ static int user_field_size(const char *type) return -EINVAL; } +static void user_event_destroy_validators(struct user_event *user) +{ + struct user_event_validator *validator, *next; + struct list_head *head = &user->validators; + + list_for_each_entry_safe(validator, next, head, link) { + list_del(&validator->link); + kfree(validator); + } +} + static void user_event_destroy_fields(struct user_event *user) { struct ftrace_event_field *field, *next; @@ -229,13 +251,43 @@ static int user_event_add_field(struct user_event *user, const char *type, const char *name, int offset, int size, int is_signed, int filter_type) { + struct user_event_validator *validator; struct ftrace_event_field *field; + int validator_flags = 0; field = kmalloc(sizeof(*field), GFP_KERNEL); if (!field) return -ENOMEM; + if (str_has_prefix(type, "__data_loc ")) + goto add_validator; + + if (str_has_prefix(type, "__rel_loc ")) { + validator_flags |= VALIDATOR_REL; + goto add_validator; + } + + goto add_field; + +add_validator: + if (strstr(type, "char") != 0) + validator_flags |= VALIDATOR_ENSURE_NULL; + + validator = kmalloc(sizeof(*validator), GFP_KERNEL); + + if (!validator) { + kfree(field); + return -ENOMEM; + } + + validator->flags = validator_flags; + validator->offset = offset; + + /* Want sequential access when validating */ + list_add_tail(&validator->link, &user->validators); + +add_field: field->type = type; field->name = name; field->offset = offset; @@ -245,6 +297,12 @@ static int user_event_add_field(struct user_event *user, const char *type, list_add(&field->link, &user->fields); + /* + * Min size from user writes that are required, this does not include + * the size of trace_entry (common fields). + */ + user->min_size = (offset + size) - sizeof(struct trace_entry); + return 0; } @@ -516,6 +574,7 @@ static int destroy_user_event(struct user_event *user) clear_bit(user->index, page_bitmap); hash_del(&user->node); + user_event_destroy_validators(user); kfree(user->call.print_fmt); kfree(EVENT_NAME(user)); kfree(user); @@ -537,15 +596,49 @@ static struct user_event *find_user_event(char *name, u32 *outkey) return NULL; } +static int user_event_validate(struct user_event *user, void *data, int len) +{ + struct list_head *head = &user->validators; + struct user_event_validator *validator; + void *pos, *end = data + len; + u32 loc, offset, size; + + list_for_each_entry(validator, head, link) { + pos = data + validator->offset; + + /* Already done min_size check, no bounds check here */ + loc = *(u32 *)pos; + offset = loc & 0xffff; + size = loc >> 16; + + if (likely(validator->flags & VALIDATOR_REL)) + pos += offset + sizeof(loc); + else + pos = data + offset; + + pos += size; + + if (unlikely(pos > end)) + return -EFAULT; + + if (likely(validator->flags & VALIDATOR_ENSURE_NULL)) + if (unlikely(*(char *)(pos - 1) != 0)) + return -EFAULT; + } + + return 0; +} + /* * Writes the user supplied payload out to a trace file. */ static void user_event_ftrace(struct user_event *user, struct iov_iter *i, - void *tpdata) + void *tpdata, bool *faulted) { struct trace_event_file *file; struct trace_entry *entry; struct trace_event_buffer event_buffer; + size_t size = sizeof(*entry) + i->count; file = (struct trace_event_file *)tpdata; @@ -555,19 +648,25 @@ static void user_event_ftrace(struct user_event *user, struct iov_iter *i, return; /* Allocates and fills trace_entry, + 1 of this is data payload */ - entry = trace_event_buffer_reserve(&event_buffer, file, - sizeof(*entry) + i->count); + entry = trace_event_buffer_reserve(&event_buffer, file, size); if (unlikely(!entry)) return; - if (unlikely(!copy_nofault(entry + 1, i->count, i))) { - __trace_event_discard_commit(event_buffer.buffer, - event_buffer.event); - return; - } + if (unlikely(!copy_nofault(entry + 1, i->count, i))) + goto discard; + + if (!list_empty(&user->validators) && + unlikely(user_event_validate(user, entry, size))) + goto discard; trace_event_buffer_commit(&event_buffer); + + return; +discard: + *faulted = true; + __trace_event_discard_commit(event_buffer.buffer, + event_buffer.event); } #ifdef CONFIG_PERF_EVENTS @@ -622,7 +721,7 @@ static void user_event_bpf(struct user_event *user, struct iov_iter *i) * Writes the user supplied payload out to perf ring buffer or eBPF program. */ static void user_event_perf(struct user_event *user, struct iov_iter *i, - void *tpdata) + void *tpdata, bool *faulted) { struct hlist_head *perf_head; @@ -645,14 +744,21 @@ static void user_event_perf(struct user_event *user, struct iov_iter *i, perf_fetch_caller_regs(regs); - if (unlikely(!copy_nofault(perf_entry + 1, i->count, i))) { - perf_swevent_put_recursion_context(context); - return; - } + if (unlikely(!copy_nofault(perf_entry + 1, i->count, i))) + goto discard; + + if (!list_empty(&user->validators) && + unlikely(user_event_validate(user, perf_entry, size))) + goto discard; perf_trace_buf_submit(perf_entry, size, context, user->call.event.type, 1, regs, perf_head, NULL); + + return; +discard: + *faulted = true; + perf_swevent_put_recursion_context(context); } } #endif @@ -967,6 +1073,7 @@ static int user_event_parse(char *name, char *args, char *flags, INIT_LIST_HEAD(&user->class.fields); INIT_LIST_HEAD(&user->fields); + INIT_LIST_HEAD(&user->validators); user->tracepoint.name = name; @@ -1015,6 +1122,7 @@ static int user_event_parse(char *name, char *args, char *flags, return 0; put_user: user_event_destroy_fields(user); + user_event_destroy_validators(user); kfree(user); return ret; } @@ -1072,6 +1180,9 @@ static ssize_t user_events_write_core(struct file *file, struct iov_iter *i) if (unlikely(user == NULL)) return -ENOENT; + if (unlikely(i->count < user->min_size)) + return -EINVAL; + tp = &user->tracepoint; /* @@ -1083,10 +1194,13 @@ static ssize_t user_events_write_core(struct file *file, struct iov_iter *i) user_event_func_t probe_func; struct iov_iter copy; void *tpdata; + bool faulted; if (unlikely(iov_iter_fault_in_readable(i, i->count))) return -EFAULT; + faulted = false; + rcu_read_lock_sched(); probe_func_ptr = rcu_dereference_sched(tp->funcs); @@ -1096,11 +1210,14 @@ static ssize_t user_events_write_core(struct file *file, struct iov_iter *i) copy = *i; probe_func = probe_func_ptr->func; tpdata = probe_func_ptr->data; - probe_func(user, ©, tpdata); + probe_func(user, ©, tpdata, &faulted); } while ((++probe_func_ptr)->func); } rcu_read_unlock_sched(); + + if (unlikely(faulted)) + return -EFAULT; } return ret;
Add validation to ensure data is at or greater than the min size for the fields of the event. If a dynamic array is used and is a type of char, ensure null termination of the array exists. Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com> --- kernel/trace/trace_events_user.c | 147 +++++++++++++++++++++++++++---- 1 file changed, 132 insertions(+), 15 deletions(-)