[pulseaudio-discuss] [PATCH v2 00/12] Introduce memfd support

Fri Feb 12 10:57:00 UTC 2016

On 2016-02-12 00:59, Ahmed S. Darwish wrote:
> Hello!

Hi and thanks for working on it!

Having skimmed through the patches, I'm still not convinced w r t 
auto-send mechanism of the memfds.

E g, consider we have clients 1 and 2, and client 1 plays back audio to 
a sink. Client 2 monitors the sink-input of client 1 (like parec does if 
you specify --monitor-stream). Could this cause the memfd used to 
communicate with client 1 to also be shared with client 2? If so, that's 
a security breach.

The mechanism seems fragile to such breaches but I'm not sure, maybe I'm 
wrong and you can explain what would happen instead?

>
> The all-improved memfd patch series ;-)
>
> ==> v1 cover letter:
>
> Memfd is a simple memory sharing mechanism, added by the systemd/kdbus
> developers, to share pages between processes in an anonymous, no
> global registry needed, no mount-point required, relatively secure,
> manner.
>
> This patch series introduces memfd support to PulseAudio, laying out
> the necessary (but not yet sufficient) groundwork for sandboxing,
> protecting PulseAudio from its clients, and protecting clients from
> each other.
>
> Memfd support is added in quite a transparent manner, respecting
> current PA mechanisms and abstractions. The lower-level layers are
> properly refactored and extended: the srbchannel communication path
> is transformed to memfds by only changing a single line of code.
>
> ==> v2 changes:
>
> - All pools (srbchannel + client audio data mempool + global mempool)
>    now use memfds by default. An empty /dev/shm FTW! ;-)
>
> - Design issues pointed by David now fixed, leading to a much smaller
>    code.. New memfd implementation now shares almost all of the
>    existing posix SHM code paths.
>
> - Heavy focus on not leaking any file descriptors throughout the
>    code. Every fd, sent and received, has a clear owner responsible
>    for closing it at appropriate times.
>
> - If a mempool is memfd-backed, we now require registering it with
>    the pstream before sending any blocks it covers. This is done to
>    to minimize fd passing overhead and the possibility of fd leaks.
>
> - A new, special, pstream packet type is introduced. That special
>    packet can be sent anytime during the pstrem's lifetime and is
>    used for creating on demand SHM ID to memfd mappings.
>
> - All issues pointed by Tanu's very detailed review (vacuuming,
>    memfd enablement over pstream, protocol changes, etc.) now fixed
>
> - Testing! I've been heavily using these patches now for three
>    weeks without visible issues.
>
> ==> references:
>
> - v1 submission
>    http://thread.gmane.org/gmane.comp.audio.pulseaudio.general/24110
>
> - memfd_create(2), David Herrmann [memfd author]
>    https://dvdhrm.wordpress.com/2014/06/10/memfd_create2/
>
> - xdg-app
>    https://wiki.gnome.org/Projects/SandboxedApps
>
> ==> diffstat:
>
> Ahmed S. Darwish (12):
>    pulsecore: Cache daemon shm size inside pa_core
>    pulsecore: srbchannel: Introduce per-client SHM files
>    pulsecore: Transform pa_mempool_new() into a factory method
>    pulsecore: Split pa_shm mempool backend into pa_shm and pa_privatemem
>    pulsecore: SHM: Introduce memfd support
>    pulsecore: memexport/memimport: Support memfd blocks
>    pulsecore, tests: Use new memexport/memimport API
>    pulsecore: Specially mark global mempools
>    pulsecore: pstream: Support memfd blocks transport
>    srbchannel: Use memfd transport by default; pump protocol version
>    pulse: client audio: Use memfd transport by default
>    core: Use memfd transport by default
>
>   PROTOCOL                         |  13 ++
>   configure.ac                     |  21 +++-
>   man/pulse-client.conf.5.xml.in   |   5 +
>   man/pulse-daemon.conf.5.xml.in   |   5 +
>   man/pulseaudio.1.xml.in          |  11 ++
>   shell-completion/bash/pulseaudio |   4 +-
>   shell-completion/zsh/_pulseaudio |   1 +
>   src/Makefile.am                  |   7 ++
>   src/daemon/cmdline.c             |  13 +-
>   src/daemon/daemon-conf.c         |   1 +
>   src/daemon/daemon-conf.h         |   1 +
>   src/daemon/main.c                |   4 +-
>   src/pulse/client-conf.c          |   1 +
>   src/pulse/client-conf.h          |   2 +-
>   src/pulse/context.c              |  48 +++++++-
>   src/pulse/internal.h             |   2 +
>   src/pulsecore/core.c             |  31 +++--
>   src/pulsecore/core.h             |  13 +-
>   src/pulsecore/mem.h              |  67 ++++++++++
>   src/pulsecore/memblock.c         | 245 ++++++++++++++++++++++++++++++++-----
>   src/pulsecore/memblock.h         |  18 ++-
>   src/pulsecore/memfd-wrappers.h   |  68 +++++++++++
>   src/pulsecore/privatemem.c       |  78 ++++++++++++
>   src/pulsecore/privatemem.h       |  35 ++++++
>   src/pulsecore/protocol-native.c  |  92 ++++++++++++--
>   src/pulsecore/pstream.c          | 257 +++++++++++++++++++++++++++++++++++----
>   src/pulsecore/pstream.h          |   2 +
>   src/pulsecore/shm.c              | 247 +++++++++++++++++++------------------
>   src/pulsecore/shm.h              |  33 ++++-
>   src/tests/cpu-mix-test.c         |   2 +-
>   src/tests/lfe-filter-test.c      |   2 +-
>   src/tests/mcalign-test.c         |   2 +-
>   src/tests/memblock-test.c        |  15 +--
>   src/tests/memblockq-test.c       |   2 +-
>   src/tests/mix-test.c             |   2 +-
>   src/tests/remix-test.c           |   2 +-
>   src/tests/resampler-test.c       |   2 +-
>   src/tests/srbchannel-test.c      |   2 +-
>   38 files changed, 1119 insertions(+), 237 deletions(-)
>   create mode 100644 src/pulsecore/mem.h
>   create mode 100644 src/pulsecore/memfd-wrappers.h
>   create mode 100644 src/pulsecore/privatemem.c
>   create mode 100644 src/pulsecore/privatemem.h
>
> Regards,
>
> --
> Darwish
> http://darwish.chasingpointers.com
>

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic