Re: [PATCH v3 1/2] dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds

[Sumit Semwal]

Hello Baineng,

On Mon, 27 Jul 2026 at 08:56, 寿柏能 <shoubaineng@xxxxxxxxx> wrote:
>
> Ping. Any chance this can be picked up?

Thank you for the patch.

While discussing with T J, we realised that adding a selftest for this
would also be helpful. Could you please re-submit with a selftest
added?
>
> Baineng

Thanks and best regards,
Sumit.

>
> 寿柏能 <shoubaineng@xxxxxxxxx> 于2026年7月15日周三 10:04写道：
>>
>> Hi David
>>
>> The rollback here is the simplest possible case (put_unused_fd +
>> dma_buf_put), not the complex sockopt scenario you describe.
>>
>> Baineng
>>
>> David Laight <david.laight.linux@xxxxxxxxx> 于2026年7月14日周二 22:33写道：
>>>
>>> On Tue, 14 Jul 2026 21:38:07 +0800
>>> 寿柏能 <shoubaineng@xxxxxxxxx> wrote:
>>>
>>> > Hi David,
>>> >
>>> > Thanks for the feedback.
>>> >
>>> > The concern is not just about the EFAULT return — it's about the race
>>> > window between fd_install() and copy_to_user().  Once fd_install()
>>> > returns, the fd is immediately observable by other threads in the same
>>> > process (via /proc/self/fd, SCM_RIGHTS, etc.), even before
>>> > copy_to_user() has a chance to fail.  The triggering condition is a
>>> > deliberate mprotect() flip, not a corrupted heap.
>>>
>>> That is what makes doing the close wrong.
>>> But that is a program aggressively trying to hit the timing window,
>>> not a normal program that has managed to pass an invalid pointer.
>>> The most likely reason for a real program passing an invalid pointer
>>> is a corrupted heap (assuming the stupid coding errors are fixed).
>>>
>>> It is really no different from the sockopt code that receives
>>> SCM_RIGHTS messages.
>>> In that case once you've removed the FILE from the socket (or similar)
>>> you really don't want to have to put it back because the write to the
>>> sockopt buffer or length field fails.
>>> The chance of correctly reverting the kernel state is small - and won't
>>> be tested.
>>>
>>>         David
>>>
>>> >
>>> > The fix itself is small and follows the standard kernel idiom:
>>> > get_unused_fd_flags() reserves the fd without publishing it, so the
>>> > window between reservation and install is entirely under kernel control.
>>> >
>>> > Baineng
>>> >
>>> > David Laight <david.laight.linux@xxxxxxxxx> 于2026年7月14日周二 21:14写道：
>>> >
>>> > > On Tue, 14 Jul 2026 19:46:53 +0800
>>> > > Baineng Shou <shoubaineng@xxxxxxxxx> wrote:
>>> > >
>>> > > > DMA_HEAP_IOCTL_ALLOC allocates a dma-buf and installs an fd into the
>>> > > > caller's fd table via dma_buf_fd() -> fd_install() before
>>> > > > dma_heap_ioctl() copies the result back to userspace.  If the trailing
>>> > > > copy_to_user() fails, userspace never learns the fd number, but the
>>> > > > fd (and the underlying dma-buf reference) are already visible to
>>> > > > other threads in the same process and are leaked for the lifetime of
>>> > > > the process.
>>> > > >
>>> > > > The obvious "close it on the failure path" fix is unsafe: once
>>> > > > fd_install() has run, another thread can already dup() the fd, send
>>> > > > it via SCM_RIGHTS, or close() it and let its number be reused, so a
>>> > > > subsequent close_fd() from the ioctl path can operate on an unrelated
>>> > > > file.  This was pointed out by Christian König on v1 [1].
>>> > > ...
>>> > >
>>> > > My 2c:
>>> > >
>>> > > The other option is just to leave it as a 'problem for user space'.
>>> > > No reasonable program is going to handle the EFAULT return by doing
>>> > > anything other than exiting.
>>> > > Even getting an EFAULT is really an indication that the application
>>> > > is already in a real mess - most likely with a badly corrupted heap.
>>> > >
>>> > > Anything else leaves error recovery code in the kernel that is pretty
>>> > > much never executed and open to a variety of bugs.
>>> > > While the recovery here is probably ok, there are some sockopt calls
>>> > > where it is all more complicated.
>>> > >
>>> > >         David
>>> > >
>>>