On Wed, Oct 06, 2021 at 12:54:41PM -0400, Steven Rostedt wrote:
> > Psuedo code example of typical usage:
> > page_fd = open("user_events_mmap", O_RDWR);
> > page_data = mmap(NULL, PAGE_SIZE, PROT_READ, MAP_SHARED, page_fd, 0);
> >
> > data_fd = open("user_events_data", O_RDWR);
> > data_id = ioctl(data_fd, DIAG_IOCSREG, "test");
> >
> > if (page_data[data_id]) write(data_fd, &payload, sizeof(payload));
>
> What is the type of "page_data". I'd like to test it before accepting it.
>
> From playing around, I see that page_data is of type char *.
Yes, it is char *. I'll make this clear in the next patch version
description.
> > +/* Bits 0-6 are for known probe types, Bit 7 is for unknown probes */
> > +#define EVENT_BIT_FTRACE 0
> > +#define EVENT_BIT_PERF 1
> > +#define EVENT_BIT_OTHER 7
> > +
> > +#define EVENT_STATUS_FTRACE (1 << EVENT_BIT_FTRACE)
> > +#define EVENT_STATUS_PERF (1 << EVENT_BIT_PERF)
> > +#define EVENT_STATUS_OTHER (1 << EVENT_BIT_OTHER)
...
> > +#define DIAG_IOC_MAGIC '*'
> > +#define DIAG_IOCSREG _IOW(DIAG_IOC_MAGIC, 0, char*)
> > +#define DIAG_IOCSDEL _IOW(DIAG_IOC_MAGIC, 1, char*)
> > +#define DIAG_IOCQLOCOFFSET _IO(DIAG_IOC_MAGIC, 2)
>
> These obviously will need to go into a user abi header file.
>
Yes, I'm glad you mentioned it. I wasn't entirely sure where it should
live. Is there precedent on where to put these so they span both kernel
and user for discovery / distribution?
> > +
> > +static char *register_page_data;
> > +
> > +static DEFINE_HASHTABLE(register_table, 4);
> > +static DECLARE_BITMAP(page_bitmap, MAX_EVENTS);
> > +
> > +struct user_event {
> > + struct tracepoint tracepoint;
> > + struct trace_event_call call;
> > + struct trace_event_class class;
> > + struct dyn_event devent;
> > + struct hlist_node node;
> > + atomic_t refs;
> > + int index;
> > + char *args;
> > +};
> > +
> > +#ifdef CONFIG_PERF_EVENTS
> > +struct user_bpf_context {
> > + int udatalen;
> > + const char __user *udata;
> > +};
> > +#endif
> > +
> > +typedef void (*user_event_func_t) (struct user_event *user,
> > + const char __user *udata,
> > + size_t udatalen, void *tpdata);
> > +
> > +static int register_user_event(char *name, char *args,
> > + struct user_event **newuser);
> > +
>
> [..]
>
Is the ask here to get user_bpf_context definition also into a user ABI
header? (I took it as that).
> > +static int __init trace_events_user_init(void)
> > +{
> > + int ret;
> > +
> > + /* Zero all bits beside 0 (which is reserved for failures) */
> > + bitmap_zero(page_bitmap, MAX_EVENTS);
> > + set_bit(0, page_bitmap);
> > +
> > + register_page_data = kmalloc(MAX_EVENTS, GFP_KERNEL);
>
> You want "kzalloc" here. Because when I read the map without adding
> anything, I get:
>
> printf("%lx\n", *(unsigned long *)page_data);
>
> Produces:
>
> ffffffff9065004e
>
> But if I convert it to kzalloc() it gives me:
>
> 0
>
> Thus, you are exposing stale memory. If you want to expose this to
> non-admin users, this is a major security leak.
>
> -- Steve
>
Oops, sorry about that!
Thanks,
-Beau
|