On Wed, Nov 22, 2023 at 10:50:06PM +0100, Jiri Olsa wrote:
> On Mon, Nov 20, 2023 at 10:04:16AM -0800, Yonghong Song wrote:
>
> SNIP
>
> > > +static int bpf_uprobe_multi_link_fill_link_info(const struct bpf_link *link,
> > > + struct bpf_link_info *info)
> > > +{
> > > + u64 __user *uref_ctr_offsets = u64_to_user_ptr(info->uprobe_multi.ref_ctr_offsets);
> > > + u64 __user *ucookies = u64_to_user_ptr(info->uprobe_multi.cookies);
> > > + u64 __user *uoffsets = u64_to_user_ptr(info->uprobe_multi.offsets);
> > > + u64 __user *upath = u64_to_user_ptr(info->uprobe_multi.path);
> > > + u32 upath_size = info->uprobe_multi.path_size;
> > > + struct bpf_uprobe_multi_link *umulti_link;
> > > + u32 ucount = info->uprobe_multi.count;
> > > + int err = 0, i;
> > > + long left;
> > > +
> > > + if (!upath ^ !upath_size)
> > > + return -EINVAL;
> > > +
> > > + if ((uoffsets || uref_ctr_offsets || ucookies) && !ucount)
> > > + return -EINVAL;
> > > +
> > > + umulti_link = container_of(link, struct bpf_uprobe_multi_link, link);
> > > + info->uprobe_multi.count = umulti_link->cnt;
> > > + info->uprobe_multi.flags = umulti_link->flags;
> > > + info->uprobe_multi.pid = umulti_link->task ?
> > > + task_pid_nr_ns(umulti_link->task, task_active_pid_ns(current)) : 0;
> > > +
> > > + if (upath) {
> > > + char *p, *buf;
> > > +
> > > + upath_size = min_t(u32, upath_size, PATH_MAX);
> > > +
> > > + buf = kmalloc(upath_size, GFP_KERNEL);
> > > + if (!buf)
> > > + return -ENOMEM;
> > > + p = d_path(&umulti_link->path, buf, upath_size);
> > > + if (IS_ERR(p)) {
> > > + kfree(buf);
> > > + return -ENOSPC;
> >
> > Should we just return PTR_ERR(p)? In d_path, it is possible that
> > -ENAMETOOLONG is returned. But path->dentry->d_op->d_dname() might
> > return a different error reason than -ENAMETOOLONG or -ENOSPC?
>
> true, will change
>
> >
> > > + }
> > > + upath_size = buf + upath_size - p;
> > > + left = copy_to_user(upath, p, upath_size);
> >
> > Here, the data copied to user may contain more than
> > actual path itself. I am okay with this since this
> > is not in critical path. But early buf allocation is using
> > kmalloc whose content could be arbitrary. Should we
> > use kzalloc for the above 'buf' allocation?
>
> good catch, will use kzalloc
hum, actually.. after checking d_path IIUC it copies into the end of buffer,
so I can't see this code copying more data to user buffer
jirka
