Re: [PATCH 1/2] dma-buf: Add an API for exporting sync files (v13)

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, May 05, 2022 at 10:27:39AM +0200, Christian König wrote:
> Am 05.05.22 um 10:10 schrieb Jason Ekstrand:
> > On Thu, May 5, 2022 at 1:25 AM Christian König
> > <christian.koenig@amd.com> wrote:
> > 
> >     [SNIP]
> >     > +     fd_install(fd, sync_file->file);
> >     > +
> >     > +     arg.fd = fd;
> >     > +     if (copy_to_user(user_data, &arg, sizeof(arg)))
> >     > +             return -EFAULT;
> > 
> >     I know we had that discussion before, but I'm not 100% any more
> >     what the
> >     outcome was.
> > 
> >     The problem here is that when the copy_to_user fails we have a file
> >     descriptor which is valid, but userspace doesn't know anything
> >     about it.
> > 
> >     I only see a few possibilities here:
> >     1. Keep it like this and just assume that a process which you
> >     can't copy
> >     the fd to is also dying (a bit to much assumption for my taste).
> > 
> >     2. Close the file descriptor when this happens (not ideal either).
> > 
> >     3. Instead of returning the fd in the parameter structure return
> >     it as
> >     IOCTL result.
> > 
> >     Number 3 is what drm_prime_handle_to_fd_ioctl() is doing as well and
> >     IIRC we said that this is probably the best option.
> > 
> > 
> > I don't have a strong preference here, so I'll go with whatever in the
> > end but let me at least explain my reasoning.  First, this was based on
> > the FD import/export in syncobj which stuffs the FD in the args struct. 
> > If `copy_to_user` is a problem here, it's a problem there as well. 
> > Second, the only way `copy_to_user` can fail is if the client gives us a
> > read-only page or somehow manages to race removing the page from their
> > address space (via unmap(), for instance) with this ioctl.  Both of
> > those seem like pretty serious client errors to me.  That, or the client
> > is in the process of dying, in which case we really don't care.
> 
> Yeah, I know about that copy_to_user() issue in the syncobj and also some
> driver specific handling.
> 
> That's why we discussed this before and IIRC somebody indeed ran into an
> issue with -EFAULT and that was the reason all this bubbled up.
> 
> I don't have a strong preference either, but I think we should try to learn
> from previous mistakes and design new interfaces based on such experience.

We have this in a bunch of places (like execbuf tail handling after
drm_sched_job_push()) and I think what we commonly do is just try to clean
up the mess a bit and fail.

I think what you could do here is do the copy_to_user before you do the
fd_install, and if the copy_to_user fails you just clean up everything and
fail. That just means there's a small window where userspace has an fd
reserve that didn't end up being used, but also in real apps this just
never matters.

Leaking the fd is maybe not the best option, but meh.
-Daniel

> 
> Christian.
> 
> > 
> > --Jason
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch