Messages in this thread Patch in this message | | | Date | Fri, 7 Jan 2022 08:32:52 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH v8 11/12] user_events: Validate user payloads for size and null termination |
| |
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
-- Masami Hiramatsu <mhiramat@kernel.org>
| |