Re: [PATCH] dmabuf: fix dmabuf file poll uaf issue

[Rob Clark]

On Tue, May 7, 2024 at 11:17 AM T.J. Mercier <tjmercier@xxxxxxxxxx> wrote:
>
> On Tue, May 7, 2024 at 7:04 AM Christian König <christian.koenig@xxxxxxx> wrote:
> >
> > Am 07.05.24 um 15:39 schrieb Daniel Vetter:
> > > On Tue, May 07, 2024 at 12:10:07PM +0200, Christian König wrote:
> > >> Am 06.05.24 um 21:04 schrieb T.J. Mercier:
> > >>> On Mon, May 6, 2024 at 2:30 AM Charan Teja Kalla
> > >>> <quic_charante@xxxxxxxxxxx> wrote:
> > >>>> Hi TJ,
> > >>>>
> > >>>> Seems I have got answers from [1], where it is agreed upon epoll() is
> > >>>> the source of issue.
> > >>>>
> > >>>> Thanks a lot for the discussion.
> > >>>>
> > >>>> [1] https://lore.kernel.org/lkml/0000000000002d631f0615918f1e@xxxxxxxxxx/
> > >>>>
> > >>>> Thanks
> > >>>> Charan
> > >>> Oh man, quite a set of threads on this over the weekend. Thanks for the link.
> > >> Yeah and it also has some interesting side conclusion: We should probably
> > >> tell people to stop using DMA-buf with epoll.
> > >>
> > >> The background is that the mutex approach epoll uses to make files disappear
> > >> from the interest list on close results in the fact that each file can only
> > >> be part of a single epoll at a time.
> > >>
> > >> Now since DMA-buf is build around the idea that we share the buffer
> > >> representation as file between processes it means that only one process at a
> > >> time can use epoll with each DMA-buf.
> > >>
> > >> So for example if a window manager uses epoll everything is fine. If a
> > >> client is using epoll everything is fine as well. But if *both* use epoll at
> > >> the same time it won't work.
> > >>
> > >> This can lead to rather funny and hard to debug combinations of failures and
> > >> I think we need to document this limitation and explicitly point it out.
> > > Ok, I tested this with a small C program, and you're mixing things up.
> > > Here's what I got
> > >
> > > - You cannot add a file twice to the same epoll file/fd. So that part is
> > >    correct, and also my understanding from reading the kernel code.
> > >
> > > - You can add the same file to two different epoll file instaces. Which
> > >    means it's totally fine to use epoll on a dma_buf in different processes
> > >    like both in the compositor and in clients.
> >
> > Ah! Than I misunderstood that comment in the discussion. Thanks for
> > clarifying that.
> >
> > >
> > > - Substantially more entertaining, you can nest epoll instances, and e.g.
> > >    add a 2nd epoll file as an event to the first one. That way you can add
> > >    the same file to both epoll fds, and so end up with the same file
> > >    essentially being added twice to the top-level epoll file. So even
> > >    within one application there's no real issue when e.g. different
> > >    userspace drivers all want to use epoll on the same fd, because you can
> > >    just throw in another level of epoll and it's fine again and you won't
> > >    get an EEXISTS on EPOLL_CTL_ADD.
> > >
> > >    But I also don't think we have this issue right now anywhere, since it's
> > >    kinda a general epoll issue that happens with any duplicated file.
> >
> > I actually have been telling people to (ab)use the epoll behavior to
> > check if two file descriptors point to the same underlying file when
> > KCMP isn't available.
> >
> > Some environments (Android?) disable KCMP because they see it as
> > security problem.
> >
> Didn't know about this so I checked. Our kernel has CONFIG_KCMP, but
> seccomp does look like it's blocking kcmp for apps.
> https://cs.android.com/android/platform/superproject/main/+/main:bionic/libc/SYSCALLS.TXT

Getting a bit off the original topic, but fwiw this is what CrOS did
for CONFIG_KCMP:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3501193

Ie. allow the kcmp syscall, but block type == KCMP_SYSVSEM, which was
the more specific thing that android wanted to block.

BR,
-R