[alsa-devel] [RFC PATCH] ALSA: core: Add DMA share buffer support
This patch adds dma share buffer support, which allows a dma-buf to be shared between processes by passing file descriptors between them, allowing multiple processes to cooperate in filling the DMA buffer used by the audio hardware without the need to copy data around. This reduces both latency and CPU load, especially in configurations where only one application is playing audio and so context switches can be avoided.
In userspace, we can use ioctl:SNDRV_PCM_IOCTL_DMABUF_EXPORT to export one dma buffer to a PCM, and use ioctl:SNDRV_PCM_IOCTL_DMABUF_ATTACH and ioctl:SNDRV_PCM_IOCTL_DMABUF_DETACH to attach or detach one device to the dma buffer. Morevoer we can use dma buffer ioctl:DMA_BUF_IOCTL_SYNC to guarantee the cache coherency of the DMA buffer.
You can refer to below patches [1] created by Leo Yan to use the dma buffer in userspace.
[1] https://github.com/tinyalsa/tinyalsa/pull/126
Welcome any comments. Thanks.
Signed-off-by: Baolin Wang baolin.wang@linaro.org --- Hi,
Before sending to ALSA mailing list, we had some internal discussion off line, so I posted them to follow up.
1. One issue proposed by Srinivas Kandagatla, he proposed one use case example: DSP1 pre-process(compress-to-pcm) the buffers, pass the fd to DSP2/audio-ip to play pcm data. The dmabuf exporter (DSP1) is a different device than audio device in this instance, so the DSP1 device cannot call snd_pcm_dmabuf_export() to export one dma buffer and pass to the DSP2/audio-ip.
Our original design is, the dma buffer should be associated with one PCM device, since different PCM devices may need different buffer types (see SNDRV_DMA_TYPE_XXX) to play PCM data, and need consider the PCM device's coherent capability for DMA memory. My concern is, if we let other non-audio device to allocate one buffer and export it, how to guarantee the dma buffer can be used by PCM device and have the same coherent capability with the PCM device.
So I am not sure for this issue and need more suggestion to get a consensus.
2. Second question raised by Srinivas Kandagatla, he wondered: "Am still unclear on the whole idea making a audio device dmabuf exporter in Linux systems, unless you are sharing the dmabuf fd with other devices.
If you are working with just one device we could just live with mmap, isn't it?"
Mark Brown gave the answer: "The issue is permissions management. We've got a sound server that owns all the sound hardware and needs to be able to retain administrative control over it which means that permissions for the sound devices are locked down to it. For performance reasons on systems where it's practical (eg, where there's multiple audio streams the system can use to play data to the hardware) we want to be able to have that server allow other applications with lower permissions to stream data to/from the hardware without going through it. The applications can't mmap() the device directly as that's too painful to do that securely from a permission management point of view so we want to be able to have the sound server do the mmap() then hand the mapped buffer off to the application for it to use via a FD over a pipe. That way the only thing with direct access to the devices is the sound server but the clients have a data path that doesn't need to bounce through another process.
Practically speaking the only use case I can think of in an audio context is for one reader and one writer (AIUI Android is envisioning this as one hardware and one software), it's difficult to see how you could usefully chain multiple in place transformations together in the way that you can with video applications but I'm possibly not thinking of something here." --- include/sound/pcm.h | 2 + include/sound/pcm_dmabuf.h | 29 +++ include/uapi/sound/asound.h | 3 + sound/core/Kconfig | 4 + sound/core/Makefile | 1 + sound/core/pcm.c | 2 + sound/core/pcm_compat.c | 3 + sound/core/pcm_dmabuf.c | 531 +++++++++++++++++++++++++++++++++++++++++++ sound/core/pcm_native.c | 38 ++++ 9 files changed, 613 insertions(+) create mode 100644 include/sound/pcm_dmabuf.h create mode 100644 sound/core/pcm_dmabuf.c
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index c47d9b4..d353e55 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -30,6 +30,7 @@ #include <linux/mm.h> #include <linux/bitops.h> #include <linux/pm_qos.h> +#include <linux/dma-buf.h>
#define snd_pcm_substream_chip(substream) ((substream)->private_data) #define snd_pcm_chip(pcm) ((pcm)->private_data) @@ -455,6 +456,7 @@ struct snd_pcm_substream { size_t buffer_bytes_max; /* limit ring buffer size */ struct snd_dma_buffer dma_buffer; size_t dma_max; + struct dma_buf_attachment *attachment; /* -- hardware operations -- */ const struct snd_pcm_ops *ops; /* -- runtime information -- */ diff --git a/include/sound/pcm_dmabuf.h b/include/sound/pcm_dmabuf.h new file mode 100644 index 0000000..4e88c5f6 --- /dev/null +++ b/include/sound/pcm_dmabuf.h @@ -0,0 +1,29 @@ +#ifndef __SOUND_PCM_DMABUF_H +#define __SOUND_PCM_DMABUF_H + +#include <sound/pcm.h> + +#ifdef CONFIG_SND_PCM_DMABUF +int snd_pcm_dmabuf_export(struct snd_pcm_substream *substream); +int snd_pcm_dmabuf_attach(struct snd_pcm_substream *substream, int fd); +void snd_pcm_dmabuf_detach(struct snd_pcm_substream *substream); +#else +static inline int snd_pcm_dmabuf_export(struct snd_pcm_substream *substream) +{ + return -EBADF; +} + +static inline int snd_pcm_dmabuf_attach(struct snd_pcm_substream *substream, + int fd) +{ + return -ENXIO; +} + +static inline void snd_pcm_dmabuf_detach(struct snd_pcm_substream *substream) +{ + +} + +#endif + +#endif /* __SOUND_PCM_DMABUF_H */ diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 404d4b9..4770b41 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -602,6 +602,9 @@ enum { #define SNDRV_PCM_IOCTL_READN_FRAMES _IOR('A', 0x53, struct snd_xfern) #define SNDRV_PCM_IOCTL_LINK _IOW('A', 0x60, int) #define SNDRV_PCM_IOCTL_UNLINK _IO('A', 0x61) +#define SNDRV_PCM_IOCTL_DMABUF_EXPORT _IOR('A', 0x70, int) +#define SNDRV_PCM_IOCTL_DMABUF_ATTACH _IOW('A', 0x71, int) +#define SNDRV_PCM_IOCTL_DMABUF_DETACH _IO('A', 0x72)
/***************************************************************************** * * diff --git a/sound/core/Kconfig b/sound/core/Kconfig index 63b3ef9..5ee02f2 100644 --- a/sound/core/Kconfig +++ b/sound/core/Kconfig @@ -184,4 +184,8 @@ config SND_DMA_SGBUF def_bool y depends on X86
+config SND_PCM_DMABUF + def_bool y + select DMA_SHARED_BUFFER + source "sound/core/seq/Kconfig" diff --git a/sound/core/Makefile b/sound/core/Makefile index ee4a4a6..beea463 100644 --- a/sound/core/Makefile +++ b/sound/core/Makefile @@ -49,3 +49,4 @@ obj-$(CONFIG_SND_OSSEMUL) += oss/ obj-$(CONFIG_SND_SEQUENCER) += seq/
obj-$(CONFIG_SND_COMPRESS_OFFLOAD) += snd-compress.o +obj-$(CONFIG_SND_PCM_DMABUF) += pcm_dmabuf.o diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 01b9d62..7781db2 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -32,6 +32,7 @@ #include <sound/timer.h> #include <sound/control.h> #include <sound/info.h> +#include <sound/pcm_dmabuf.h>
#include "pcm_local.h"
@@ -890,6 +891,7 @@ static void snd_pcm_free_stream(struct snd_pcm_str * pstr) substream_next = substream->next; snd_pcm_timer_done(substream); snd_pcm_substream_proc_done(substream); + snd_pcm_dmabuf_detach(substream); kfree(substream); substream = substream_next; } diff --git a/sound/core/pcm_compat.c b/sound/core/pcm_compat.c index 946ab08..af24489 100644 --- a/sound/core/pcm_compat.c +++ b/sound/core/pcm_compat.c @@ -687,6 +687,9 @@ static long snd_pcm_ioctl_compat(struct file *file, unsigned int cmd, unsigned l case SNDRV_PCM_IOCTL_XRUN: case SNDRV_PCM_IOCTL_LINK: case SNDRV_PCM_IOCTL_UNLINK: + case SNDRV_PCM_IOCTL_DMABUF_EXPORT: + case SNDRV_PCM_IOCTL_DMABUF_ATTACH: + case SNDRV_PCM_IOCTL_DMABUF_DETACH: return snd_pcm_common_ioctl(file, substream, cmd, argp); case SNDRV_PCM_IOCTL_HW_REFINE32: return snd_pcm_ioctl_hw_params_compat(substream, 1, argp); diff --git a/sound/core/pcm_dmabuf.c b/sound/core/pcm_dmabuf.c new file mode 100644 index 0000000..76211ea --- /dev/null +++ b/sound/core/pcm_dmabuf.c @@ -0,0 +1,531 @@ +// SPDX-License-Identifier: GPL-2.0+ +// +// Copyright(C) 2018 Linaro Limited. All rights reserved. +// Author: Baolin Wang baolin.wang@linaro.org + +#include <linux/export.h> +#include <linux/dma-buf.h> +#include <sound/core.h> +#include <sound/pcm.h> +#include <linux/slab.h> + +struct pcm_dmabuf_attachment { + struct device *dev; + struct sg_table *sgt; + enum dma_data_direction dir; + struct list_head list; +}; + +struct pcm_dmabuf_object { + struct snd_dma_buffer *dmab; + struct dma_buf *dmabuf; + struct page **pages; + int pages_num; + struct mutex lock; + struct list_head attachments; +}; + +static int snd_pcm_map_attach(struct dma_buf *dma_buf, + struct dma_buf_attachment *attach) +{ + struct pcm_dmabuf_attachment *pcm_attach; + + pcm_attach = kzalloc(sizeof(*pcm_attach), GFP_KERNEL); + if (!pcm_attach) + return -ENOMEM; + + pcm_attach->dir = DMA_NONE; + attach->priv = pcm_attach; + + return 0; +} + +static void snd_pcm_map_detach(struct dma_buf *dma_buf, + struct dma_buf_attachment *attach) +{ + struct pcm_dmabuf_attachment *pcm_attach = attach->priv; + struct sg_table *sgt; + + if (!pcm_attach) + return; + + sgt = pcm_attach->sgt; + if (sgt) { + if (pcm_attach->dir != DMA_NONE) + dma_unmap_sg_attrs(attach->dev, sgt->sgl, + sgt->nents, + pcm_attach->dir, + DMA_ATTR_SKIP_CPU_SYNC); + + sg_free_table(sgt); + } + + kfree(sgt); + kfree(pcm_attach); + attach->priv = NULL; +} + +static struct sg_table *snd_pcm_dmabuf_to_sgt(struct pcm_dmabuf_object *obj) +{ + struct snd_dma_buffer *dmab = obj->dmab; + int type = dmab->dev.type, ret, i; + struct sg_table *sgt; + unsigned long offset; + + sgt = kmalloc(sizeof(*sgt), GFP_KERNEL); + if (!sgt) + return ERR_PTR(-ENOMEM); + + switch (type) { + case SNDRV_DMA_TYPE_CONTINUOUS: + offset = offset_in_page(dmab->area); + + for (i = 0; i < obj->pages_num; i++) { + struct page *page = + virt_to_page(dmab->area + i * PAGE_SIZE); + + obj->pages[i] = page; + } + + ret = sg_alloc_table_from_pages(sgt, obj->pages, obj->pages_num, + offset, dmab->bytes, GFP_KERNEL); + if (ret) + goto error; + + break; + +#ifdef CONFIG_GENERIC_ALLOCATOR + case SNDRV_DMA_TYPE_DEV_IRAM: +#endif + case SNDRV_DMA_TYPE_DEV: + case SNDRV_DMA_TYPE_DEV_UC: + ret = dma_get_sgtable(dmab->dev.dev, sgt, dmab->area, + dmab->addr, dmab->bytes); + if (ret) + goto error; + + break; + +#ifdef CONFIG_SND_DMA_SGBUF + case SNDRV_DMA_TYPE_DEV_SG: + case SNDRV_DMA_TYPE_DEV_UC_SG: + struct snd_sg_buf *sgbuf = dmab->private_data; + + obj->pages = sgbuf->page_table; + ret = sg_alloc_table_from_pages(sgt, obj->pages, obj->pages_num, + 0, dmab->bytes, GFP_KERNEL); + if (ret) + goto error; + + break; +#endif + + default: + ret = -ENXIO; + goto error; + } + + return sgt; + +error: + kfree(sgt); + return ERR_PTR(ret); +} + +static struct sg_table *snd_pcm_map_dmabuf(struct dma_buf_attachment *attach, + enum dma_data_direction dir) +{ + struct pcm_dmabuf_attachment *pcm_attach = attach->priv; + struct pcm_dmabuf_object *obj = attach->dmabuf->priv; + struct sg_table *sgt; + int ret; + + if (WARN_ON(dir == DMA_NONE || !pcm_attach)) + return ERR_PTR(-EINVAL); + + if (pcm_attach->dir == dir) + return pcm_attach->sgt; + + if (WARN_ON(pcm_attach->dir != DMA_NONE)) + return ERR_PTR(-EBUSY); + + sgt = snd_pcm_dmabuf_to_sgt(obj); + if (IS_ERR(sgt)) + return sgt; + + ret = dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir, + DMA_ATTR_SKIP_CPU_SYNC); + if (!ret) { + sg_free_table(sgt); + kfree(sgt); + return ERR_PTR(-ENOMEM); + } + + pcm_attach->sgt = sgt; + pcm_attach->dir = dir; + + return sgt; +} + +static void snd_pcm_unmap_dmabuf(struct dma_buf_attachment *attach, + struct sg_table *sgt, + enum dma_data_direction dir) +{ + /* Nothing need to do */ +} + +static int snd_pcm_dmabuf_mmap(struct dma_buf *dma_buf, + struct vm_area_struct *vma) +{ + struct pcm_dmabuf_object *obj = dma_buf->priv; + struct snd_dma_buffer *dmab = obj->dmab; + unsigned long addr = vma->vm_start; + unsigned long offset = vma->vm_pgoff * PAGE_SIZE; + int type = dmab->dev.type, ret, i; + struct sg_table *sgt; + struct scatterlist *sg; + + switch (type) { +#ifdef CONFIG_GENERIC_ALLOCATOR + case SNDRV_DMA_TYPE_DEV_IRAM: +#endif + case SNDRV_DMA_TYPE_DEV: + case SNDRV_DMA_TYPE_DEV_UC: + return dma_mmap_coherent(dmab->dev.dev, vma, + dmab->area, dmab->addr, + vma->vm_end - vma->vm_start); + +#ifdef CONFIG_SND_DMA_SGBUF + case SNDRV_DMA_TYPE_DEV_SG: + case SNDRV_DMA_TYPE_DEV_UC_SG: +#endif + case SNDRV_DMA_TYPE_CONTINUOUS: + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); + + sgt = snd_pcm_dmabuf_to_sgt(obj); + if (IS_ERR(sgt)) + return PTR_ERR(sgt); + + for_each_sg(sgt->sgl, sg, sgt->nents, i) { + struct page *page = sg_page(sg); + unsigned long remainder = vma->vm_end - addr; + unsigned long len = sg->length; + + if (offset >= sg->length) { + offset -= sg->length; + continue; + } else if (offset) { + page += offset / PAGE_SIZE; + len = sg->length - offset; + offset = 0; + } + + len = min(len, remainder); + ret = remap_pfn_range(vma, addr, page_to_pfn(page), len, + vma->vm_page_prot); + if (ret) + return ret; + + addr += len; + if (addr >= vma->vm_end) + return 0; + } + + default: + return -ENXIO; + } + + return 0; +} + +static void snd_pcm_dmabuf_release(struct dma_buf *dmabuf) +{ + struct pcm_dmabuf_object *obj = dmabuf->priv; + + kfree(obj->pages); + kfree(obj); +} + +static int snd_pcm_dmabuf_begin_cpu_access(struct dma_buf *dmabuf, + enum dma_data_direction direction) +{ + struct pcm_dmabuf_object *obj = dmabuf->priv; + struct snd_dma_buffer *dmab = obj->dmab; + int type = dmab->dev.type; + struct pcm_dmabuf_attachment *pcm_attach; + + switch (type) { +#ifdef CONFIG_GENERIC_ALLOCATOR + case SNDRV_DMA_TYPE_DEV_IRAM: +#endif + case SNDRV_DMA_TYPE_DEV: + case SNDRV_DMA_TYPE_DEV_UC: + /* Memory types are uncacheable, nothing need to do. */ + return 0; + +#ifdef CONFIG_SND_DMA_SGBUF + case SNDRV_DMA_TYPE_DEV_SG: + case SNDRV_DMA_TYPE_DEV_UC_SG: +#endif + case SNDRV_DMA_TYPE_CONTINUOUS: + mutex_lock(&obj->lock); + + /* + * Sync the whole DMA buffer properly in order for the CPU + * to see the most up-to-date and correct copy of the DMA + * buffer. + */ + list_for_each_entry(pcm_attach, &obj->attachments, list) { + dma_sync_sg_for_cpu(pcm_attach->dev, + pcm_attach->sgt->sgl, + pcm_attach->sgt->nents, + direction); + } + + mutex_unlock(&obj->lock); + + break; + + default: + return -ENXIO; + } + + return 0; +} + +static int snd_pcm_dmabuf_end_cpu_access(struct dma_buf *dmabuf, + enum dma_data_direction direction) +{ + struct pcm_dmabuf_object *obj = dmabuf->priv; + struct snd_dma_buffer *dmab = obj->dmab; + struct pcm_dmabuf_attachment *pcm_attach; + int type = dmab->dev.type; + + switch (type) { +#ifdef CONFIG_GENERIC_ALLOCATOR + case SNDRV_DMA_TYPE_DEV_IRAM: +#endif + case SNDRV_DMA_TYPE_DEV: + case SNDRV_DMA_TYPE_DEV_UC: + /* Memory types are uncacheable, nothing need to do. */ + return 0; + +#ifdef CONFIG_SND_DMA_SGBUF + case SNDRV_DMA_TYPE_DEV_SG: + case SNDRV_DMA_TYPE_DEV_UC_SG: +#endif + case SNDRV_DMA_TYPE_CONTINUOUS: + mutex_lock(&obj->lock); + + /* + * Sync the whole DMA buffer properly in order for the devices + * to see the most up-to-date and correct copy of the DMA + * buffer. + */ + list_for_each_entry(pcm_attach, &obj->attachments, list) { + dma_sync_sg_for_device(pcm_attach->dev, + pcm_attach->sgt->sgl, + pcm_attach->sgt->nents, + direction); + } + + mutex_unlock(&obj->lock); + + break; + + default: + return -ENXIO; + } + + return 0; +} + +static const struct dma_buf_ops snd_pcm_dmabuf_ops = { + .attach = snd_pcm_map_attach, + .detach = snd_pcm_map_detach, + .map_dma_buf = snd_pcm_map_dmabuf, + .unmap_dma_buf = snd_pcm_unmap_dmabuf, + .release = snd_pcm_dmabuf_release, + .mmap = snd_pcm_dmabuf_mmap, + .begin_cpu_access = snd_pcm_dmabuf_begin_cpu_access, + .end_cpu_access = snd_pcm_dmabuf_end_cpu_access, +}; + +/** + * snd_pcm_dmabuf_export - export one dma buffer associated with a PCM substream + * @substream: PCM substream + * + * Return: a file descriptor for the given dma buffer, otherwise a negative + * value on error. + */ +int snd_pcm_dmabuf_export(struct snd_pcm_substream *substream) +{ + struct snd_dma_buffer *dmab = &substream->dma_buffer; + DEFINE_DMA_BUF_EXPORT_INFO(exp_info); + struct pcm_dmabuf_object *obj; + int ret; + + obj = kzalloc(sizeof(*obj), GFP_KERNEL); + if (!obj) + return -ENOMEM; + + mutex_init(&obj->lock); + INIT_LIST_HEAD(&obj->attachments); + obj->dmab = dmab; + obj->pages_num = (dmab->bytes + PAGE_SIZE - 1) >> PAGE_SHIFT; + obj->pages = kcalloc(obj->pages_num, sizeof(obj->pages[0]), GFP_KERNEL); + if (!obj->pages) { + ret = -ENOMEM; + goto alloc_err; + } + + exp_info.ops = &snd_pcm_dmabuf_ops; + exp_info.size = dmab->bytes; + exp_info.flags = O_RDWR; + exp_info.priv = obj; + + obj->dmabuf = dma_buf_export(&exp_info); + if (IS_ERR(obj->dmabuf)) { + ret = PTR_ERR(obj->dmabuf); + goto export_err; + } + + ret = dma_buf_fd(obj->dmabuf, O_CLOEXEC); + if (ret < 0) + goto fd_err; + + return ret; + +fd_err: + dma_buf_put(obj->dmabuf); +export_err: + kfree(obj->pages); +alloc_err: + kfree(obj); + return ret; +} +EXPORT_SYMBOL(snd_pcm_dmabuf_export); + +/** + * snd_pcm_dmabuf_attach - attach one device to the dma buffer + * @substream: PCM substream + * @fd: file descriptor for the dma buffer + * + * Add one attachment to the dma buffer and map the scatterlist table of + * the attachment into device address space. + * + * Return: zero if attaching successfully, otherwise a negative value on error. + */ +int snd_pcm_dmabuf_attach(struct snd_pcm_substream *substream, int fd) +{ + enum dma_data_direction dir = + substream->stream == SNDRV_PCM_STREAM_PLAYBACK ? + DMA_TO_DEVICE : DMA_FROM_DEVICE; + struct snd_card *card = substream->pcm->card; + struct device *dev = card->dev; + struct dma_buf_attachment *attachment; + struct pcm_dmabuf_object *obj; + struct pcm_dmabuf_attachment *pcm_attach; + struct snd_dma_buffer *dmab; + struct dma_buf *dmabuf; + struct sg_table *sgt; + int ret; + + dmabuf = dma_buf_get(fd); + if (IS_ERR(dmabuf)) + return PTR_ERR(dmabuf); + + attachment = dma_buf_attach(dmabuf, dev); + if (IS_ERR(attachment)) { + ret = PTR_ERR(attachment); + goto err_put; + } + + sgt = dma_buf_map_attachment(attachment, dir); + if (IS_ERR(sgt)) { + ret = PTR_ERR(sgt); + goto err_detach; + } + + pcm_attach = attachment->priv; + pcm_attach->dev = dev; + INIT_LIST_HEAD(&pcm_attach->list); + obj = dmabuf->priv; + dmab = obj->dmab; + + switch (dmab->dev.type) { + case SNDRV_DMA_TYPE_CONTINUOUS: + substream->runtime->dma_area = dmab->area; + substream->runtime->dma_addr = sg_dma_address(sgt->sgl); + substream->runtime->dma_bytes = dmab->bytes; + break; + +#ifdef CONFIG_GENERIC_ALLOCATOR + case SNDRV_DMA_TYPE_DEV_IRAM: +#endif + case SNDRV_DMA_TYPE_DEV: + case SNDRV_DMA_TYPE_DEV_UC: + substream->runtime->dma_area = dmab->area; + substream->runtime->dma_addr = dmab->addr; + substream->runtime->dma_bytes = dmab->bytes; + break; + +#ifdef CONFIG_SND_DMA_SGBUF + case SNDRV_DMA_TYPE_DEV_SG: + case SNDRV_DMA_TYPE_DEV_UC_SG: + /* TODO: Do not support now */ + /* fall-through */ +#endif + + default: + ret = -ENXIO; + goto err_runtime_buf; + } + + substream->attachment = attachment; + + mutex_lock(&obj->lock); + list_add(&pcm_attach->list, &obj->attachments); + mutex_unlock(&obj->lock); + + return 0; + +err_runtime_buf: + dma_buf_unmap_attachment(attachment, sgt, dir); +err_detach: + dma_buf_detach(dmabuf, attachment); +err_put: + dma_buf_put(dmabuf); + + return ret; +} +EXPORT_SYMBOL(snd_pcm_dmabuf_attach); + +/** + * snd_pcm_dmabuf_detach - detach the given attachment from dma buffer + * @substream: PCM substream + */ +void snd_pcm_dmabuf_detach(struct snd_pcm_substream *substream) +{ + struct dma_buf_attachment *attachment = substream->attachment; + struct pcm_dmabuf_attachment *pcm_attach; + struct pcm_dmabuf_object *obj; + struct dma_buf *dmabuf; + + if (!attachment) + return; + + pcm_attach = attachment->priv; + dmabuf = attachment->dmabuf; + obj = dmabuf->priv; + + mutex_lock(&obj->lock); + list_del(&pcm_attach->list); + mutex_unlock(&obj->lock); + + dma_buf_unmap_attachment(attachment, pcm_attach->sgt, pcm_attach->dir); + dma_buf_detach(dmabuf, attachment); + dma_buf_put(dmabuf); + substream->attachment = NULL; +} +EXPORT_SYMBOL(snd_pcm_dmabuf_detach); diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index ac37a71..5fad8dc 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -33,6 +33,7 @@ #include <sound/info.h> #include <sound/pcm.h> #include <sound/pcm_params.h> +#include <sound/pcm_dmabuf.h> #include <sound/timer.h> #include <sound/minors.h> #include <linux/uio.h> @@ -2863,6 +2864,37 @@ static int snd_pcm_forward_ioctl(struct snd_pcm_substream *substream, return result < 0 ? result : 0; }
+static int snd_pcm_dmabuf_export_ioctl(struct snd_pcm_substream *substream, + int __user *_fd) +{ + int fd; + + fd = snd_pcm_dmabuf_export(substream); + if (fd < 0) + return fd; + + __put_user(fd, _fd); + return 0; +} + +static int snd_pcm_dmabuf_attach_ioctl(struct snd_pcm_substream *substream, + int __user *_fd) +{ + int fd; + + if (get_user(fd, _fd)) + return -EFAULT; + + return snd_pcm_dmabuf_attach(substream, fd); +} + +static int snd_pcm_dmabuf_detach_ioctl(struct snd_pcm_substream *substream) +{ + snd_pcm_dmabuf_detach(substream); + + return 0; +} + static int snd_pcm_common_ioctl(struct file *file, struct snd_pcm_substream *substream, unsigned int cmd, void __user *arg) @@ -2960,6 +2992,12 @@ static int snd_pcm_common_ioctl(struct file *file, return snd_pcm_rewind_ioctl(substream, arg); case SNDRV_PCM_IOCTL_FORWARD: return snd_pcm_forward_ioctl(substream, arg); + case SNDRV_PCM_IOCTL_DMABUF_EXPORT: + return snd_pcm_dmabuf_export_ioctl(substream, arg); + case SNDRV_PCM_IOCTL_DMABUF_ATTACH: + return snd_pcm_dmabuf_attach_ioctl(substream, arg); + case SNDRV_PCM_IOCTL_DMABUF_DETACH: + return snd_pcm_dmabuf_detach_ioctl(substream); } pcm_dbg(substream->pcm, "unknown ioctl = 0x%x\n", cmd); return -ENOTTY;
Dne 18.1.2019 v 05:55 Baolin Wang napsal(a):
This patch adds dma share buffer support, which allows a dma-buf to be shared between processes by passing file descriptors between them, allowing multiple processes to cooperate in filling the DMA buffer used by the audio hardware without the need to copy data around. This reduces both latency and CPU load, especially in configurations where only one application is playing audio and so context switches can be avoided.
In userspace, we can use ioctl:SNDRV_PCM_IOCTL_DMABUF_EXPORT to export one dma buffer to a PCM, and use ioctl:SNDRV_PCM_IOCTL_DMABUF_ATTACH and ioctl:SNDRV_PCM_IOCTL_DMABUF_DETACH to attach or detach one device to the dma buffer. Morevoer we can use dma buffer ioctl:DMA_BUF_IOCTL_SYNC to guarantee the cache coherency of the DMA buffer.
You can refer to below patches [1] created by Leo Yan to use the dma buffer in userspace.
[1] https://github.com/tinyalsa/tinyalsa/pull/126
Welcome any comments. Thanks.
Signed-off-by: Baolin Wang baolin.wang@linaro.org
Hi,
Before sending to ALSA mailing list, we had some internal discussion off line, so I posted them to follow up.
- One issue proposed by Srinivas Kandagatla, he proposed one use case example:
DSP1 pre-process(compress-to-pcm) the buffers, pass the fd to DSP2/audio-ip to play pcm data. The dmabuf exporter (DSP1) is a different device than audio device in this instance, so the DSP1 device cannot call snd_pcm_dmabuf_export() to export one dma buffer and pass to the DSP2/audio-ip.
Our original design is, the dma buffer should be associated with one PCM device, since different PCM devices may need different buffer types (see SNDRV_DMA_TYPE_XXX) to play PCM data, and need consider the PCM device's coherent capability for DMA memory. My concern is, if we let other non-audio device to allocate one buffer and export it, how to guarantee the dma buffer can be used by PCM device and have the same coherent capability with the PCM device.
So I am not sure for this issue and need more suggestion to get a consensus.
- Second question raised by Srinivas Kandagatla, he wondered:
"Am still unclear on the whole idea making a audio device dmabuf exporter in Linux systems, unless you are sharing the dmabuf fd with other devices.
If you are working with just one device we could just live with mmap, isn't it?"
Mark Brown gave the answer: "The issue is permissions management. We've got a sound server that owns all the sound hardware and needs to be able to retain administrative control over it which means that permissions for the sound devices are locked down to it. For performance reasons on systems where it's practical (eg, where there's multiple audio streams the system can use to play data to the hardware) we want to be able to have that server allow other applications with lower permissions to stream data to/from the hardware without going through it. The applications can't mmap() the device directly as that's too painful to do that securely from a permission management point of view so we want to be able to have the sound server do the mmap() then hand the mapped buffer off to the application for it to use via a FD over a pipe. That way the only thing with direct access to the devices is the sound server but the clients have a data path that doesn't need to bounce through another process.
Practically speaking the only use case I can think of in an audio context is for one reader and one writer (AIUI Android is envisioning this as one hardware and one software), it's difficult to see how you could usefully chain multiple in place transformations together in the way that you can with video applications but I'm possibly not thinking of something here."
Hi,
the tinyalsa implementation does not show much - it's equal to the standard mmap access for the PCM devices. Even considering the Mark's text, there must be an arbiter (sound server) which communicates with the producer or consumer to control the data flow. I really would like to see a real usage for this.
It seems to me that the only point to implement this is the permissions. We already have O_APPEND mode for the PCM file descriptor which can reuse the PCM device multiple times (mmap the buffer to multiple tasks). I would probably go in this way and add more extended permission control for the PCM device, so permissions can be restricted for the passed descriptor to the producer or the consumer task. In this way, the restricted task might reuse other control mechanism offered buy the PCM file descriptor without requesting the arbiter to do so (like read the actual position in the DMA buffer, get the audio delay or so - reduce context switches).
Thanks, Jaroslav
On Fri, Jan 18, 2019 at 10:35:44AM +0100, Jaroslav Kysela wrote:
the tinyalsa implementation does not show much - it's equal to the standard mmap access for the PCM devices. Even considering the Mark's text, there must be an arbiter (sound server) which communicates with the producer or consumer to control the data flow. I really would like to see a real usage for this.
Right, the driving force behind implementing this is Android which had been using an out of tree version of this approach based on ION but that's run into trouble due to other outside changes.
It seems to me that the only point to implement this is the permissions. We already have O_APPEND mode for the PCM file descriptor which can reuse the PCM device multiple times (mmap the buffer to multiple tasks). I would probably go in this way and add more extended permission control for the PCM device, so permissions can be restricted for the passed descriptor to the producer or the consumer task. In this way, the restricted task might reuse other control mechanism offered buy the PCM file descriptor without requesting the arbiter to do so (like read the actual position in the DMA buffer, get the audio delay or so - reduce context switches).
One concern I have with doing some ALSA-specific custom permissions thing is integration with frameworks like SELinux (they'd presumably need to learn about the ALSA specific stuff to manage it). It also seems like it's adding a lot more security sensitive interfaces and code which which will require audit and review, one of the things I really like about this approach is that it's incredibly simple from the security point of view.
On Fri, 18 Jan 2019 20:08:05 +0100, Mark Brown wrote:
On Fri, Jan 18, 2019 at 10:35:44AM +0100, Jaroslav Kysela wrote:
the tinyalsa implementation does not show much - it's equal to the standard mmap access for the PCM devices. Even considering the Mark's text, there must be an arbiter (sound server) which communicates with the producer or consumer to control the data flow. I really would like to see a real usage for this.
Right, the driving force behind implementing this is Android which had been using an out of tree version of this approach based on ION but that's run into trouble due to other outside changes.
It seems to me that the only point to implement this is the permissions. We already have O_APPEND mode for the PCM file descriptor which can reuse the PCM device multiple times (mmap the buffer to multiple tasks). I would probably go in this way and add more extended permission control for the PCM device, so permissions can be restricted for the passed descriptor to the producer or the consumer task. In this way, the restricted task might reuse other control mechanism offered buy the PCM file descriptor without requesting the arbiter to do so (like read the actual position in the DMA buffer, get the audio delay or so - reduce context switches).
One concern I have with doing some ALSA-specific custom permissions thing is integration with frameworks like SELinux (they'd presumably need to learn about the ALSA specific stuff to manage it). It also seems like it's adding a lot more security sensitive interfaces and code which which will require audit and review, one of the things I really like about this approach is that it's incredibly simple from the security point of view.
Well, I wonder what makes it more difficult by the approach Jaroslav suggested. With O_APPEND, you can just call mmap() normally, and that's all. What's the merit of dma-buf approach wrt the security?
BTW, the suggested patch seems to have a problem when the attached PCM performs hw_free. Then the mapped target will be gone while another process still mapping it. And the code looks pretty racy.
thanks,
Takashi
On Fri, Jan 18, 2019 at 08:39:32PM +0100, Takashi Iwai wrote:
Mark Brown wrote:
multiple tasks). I would probably go in this way and add more extended permission control for the PCM device, so permissions can be restricted for the passed descriptor to the producer or the consumer task. In this
One concern I have with doing some ALSA-specific custom permissions thing is integration with frameworks like SELinux (they'd presumably need to learn about the ALSA specific stuff to manage it). It also
Well, I wonder what makes it more difficult by the approach Jaroslav suggested. With O_APPEND, you can just call mmap() normally, and that's all. What's the merit of dma-buf approach wrt the security?
It was the bit about adding more extended permission control that I was worried about there, not the initial O_APPEND bit. Indeed the O_APPEND bit sounds like it might also work from the base buffer sharing point of view, I have to confess I'd not heard of that feature before (it didn't come up in the discussion when Eric raised this in Prague).
Dne 21.1.2019 v 13:40 Mark Brown napsal(a):
On Fri, Jan 18, 2019 at 08:39:32PM +0100, Takashi Iwai wrote:
Mark Brown wrote:
multiple tasks). I would probably go in this way and add more extended permission control for the PCM device, so permissions can be restricted for the passed descriptor to the producer or the consumer task. In this
One concern I have with doing some ALSA-specific custom permissions thing is integration with frameworks like SELinux (they'd presumably need to learn about the ALSA specific stuff to manage it). It also
Well, I wonder what makes it more difficult by the approach Jaroslav suggested. With O_APPEND, you can just call mmap() normally, and that's all. What's the merit of dma-buf approach wrt the security?
It was the bit about adding more extended permission control that I was worried about there, not the initial O_APPEND bit. Indeed the O_APPEND bit sounds like it might also work from the base buffer sharing point of view, I have to confess I'd not heard of that feature before (it didn't come up in the discussion when Eric raised this in Prague).
With permissions, I meant to make possible to restrict the file descriptor operations (ioctls) for the depending task (like access to the DMA buffer, synchronize it for the non-coherent platforms and maybe read/write the actual position, delay etc.). It should be relatively easy to implement using the snd_pcm_file structure.
Even with the full operation mode, it's difficult imagine what the depending task can break with the sound interface except probably annoy users with some cracks for the playback or start/stop the streaming rapidly.
Jaroslav
On Mon, Jan 21, 2019 at 03:15:43PM +0100, Jaroslav Kysela wrote:
Dne 21.1.2019 v 13:40 Mark Brown napsal(a):
It was the bit about adding more extended permission control that I was worried about there, not the initial O_APPEND bit. Indeed the O_APPEND bit sounds like it might also work from the base buffer sharing point of view, I have to confess I'd not heard of that feature before (it didn't come up in the discussion when Eric raised this in Prague).
With permissions, I meant to make possible to restrict the file descriptor operations (ioctls) for the depending task (like access to the DMA buffer, synchronize it for the non-coherent platforms and maybe read/write the actual position, delay etc.). It should be relatively easy to implement using the snd_pcm_file structure.
Right, that's what I understood you to mean. If you want to have a policy saying "it's OK to export a PCM file descriptor if it's only got permissions X and Y" the security module is going to need to know about the mechanism for setting those permissions. With dma_buf that's all a bit easier as there's less new stuff, though I've no real idea how much of a big deal that actually is.
Even with the full operation mode, it's difficult imagine what the depending task can break with the sound interface except probably annoy users with some cracks for the playback or start/stop the streaming rapidly.
In some embedded systems the ability to play back notifications clearly can be very important to system functionality. Obviously at times the main purpose of the system is to play or record audio.
I don't have a *super* strong opinion here but I'm not really a security person.
On Tue, 22 Jan 2019 21:25:35 +0100, Mark Brown wrote:
On Mon, Jan 21, 2019 at 03:15:43PM +0100, Jaroslav Kysela wrote:
Dne 21.1.2019 v 13:40 Mark Brown napsal(a):
It was the bit about adding more extended permission control that I was worried about there, not the initial O_APPEND bit. Indeed the O_APPEND bit sounds like it might also work from the base buffer sharing point of view, I have to confess I'd not heard of that feature before (it didn't come up in the discussion when Eric raised this in Prague).
With permissions, I meant to make possible to restrict the file descriptor operations (ioctls) for the depending task (like access to the DMA buffer, synchronize it for the non-coherent platforms and maybe read/write the actual position, delay etc.). It should be relatively easy to implement using the snd_pcm_file structure.
Right, that's what I understood you to mean. If you want to have a policy saying "it's OK to export a PCM file descriptor if it's only got permissions X and Y" the security module is going to need to know about the mechanism for setting those permissions. With dma_buf that's all a bit easier as there's less new stuff, though I've no real idea how much of a big deal that actually is.
There are many ways to implement such a thing, yeah. If we'd need an implementation that is done solely in the sound driver layer, I can imagine to introduce either a new ioctl or an open flag (like O_EXCL) to specify the restricted sharing. That is, a kind of master / slave model where only the master is allowed to manipulate the stream while the slave can mmap, read/write and get status.
thanks,
Takashi
Hi all,
On Wed, Jan 23, 2019 at 12:58:51PM +0100, Takashi Iwai wrote:
On Tue, 22 Jan 2019 21:25:35 +0100, Mark Brown wrote:
On Mon, Jan 21, 2019 at 03:15:43PM +0100, Jaroslav Kysela wrote:
Dne 21.1.2019 v 13:40 Mark Brown napsal(a):
It was the bit about adding more extended permission control that I was worried about there, not the initial O_APPEND bit. Indeed the O_APPEND bit sounds like it might also work from the base buffer sharing point of view, I have to confess I'd not heard of that feature before (it didn't come up in the discussion when Eric raised this in Prague).
With permissions, I meant to make possible to restrict the file descriptor operations (ioctls) for the depending task (like access to the DMA buffer, synchronize it for the non-coherent platforms and maybe read/write the actual position, delay etc.). It should be relatively easy to implement using the snd_pcm_file structure.
Right, that's what I understood you to mean. If you want to have a policy saying "it's OK to export a PCM file descriptor if it's only got permissions X and Y" the security module is going to need to know about the mechanism for setting those permissions. With dma_buf that's all a bit easier as there's less new stuff, though I've no real idea how much of a big deal that actually is.
There are many ways to implement such a thing, yeah. If we'd need an implementation that is done solely in the sound driver layer, I can imagine to introduce either a new ioctl or an open flag (like O_EXCL) to specify the restricted sharing. That is, a kind of master / slave model where only the master is allowed to manipulate the stream while the slave can mmap, read/write and get status.
I am lacking security related knowledge, especially for SELinux. So only can give background information but not sure if it's really helpful for discussion.
Android web page [1] give some information for this:
"The shared memory is referenced using a file descriptor that is generated by the ALSA driver. If the file descriptor is directly associated with a /dev/snd/ driver file, then it can be used by the AAudio service in SHARED mode. But the descriptor cannot be passed to the client code for EXCLUSIVE mode. The /dev/snd/ file descriptor would provide too broad of access to the client, so it is blocked by SELinux.
In order to support EXCLUSIVE mode, it is necessary to convert the /dev/snd/ descriptor to an anon_inode:dmabuffer file descriptor. SELinux allows that file descriptor to be passed to the client. It can also be used by the AAudioService.
An anon_inode:dmabuffer file descriptor can be generated using the Android Ion memory library."
So we work out dmabuf driver for audio buffer, the audio buffer will be exported and attached by using dma-buf framework; then we can return one file descriptor which is generated by dma-buf and this file descriptor is bound with anon inode based on dma-buf core code.
If we directly use the device node /dev/snd/ as file descriptor, even though we specify flag O_EXCL when open it, but it still is not an anon inode file descriptor. Thus this is not safe enough and will be blocked by SELinux. On the other hand, this patch wants to use dma-buf framework to provide file descriptor for the audio buffer, and this audio buffer can be one of mutiple audio buffers in the system and it can be shared to any audio client program.
Again, I have no less knowledge for SELinux so sorry if I introduce any noise at here. And very appreciate any comments for this.
Thanks, Leo Yan
Dne 23.1.2019 v 13:46 Leo Yan napsal(a):
Hi all,
On Wed, Jan 23, 2019 at 12:58:51PM +0100, Takashi Iwai wrote:
On Tue, 22 Jan 2019 21:25:35 +0100, Mark Brown wrote:
On Mon, Jan 21, 2019 at 03:15:43PM +0100, Jaroslav Kysela wrote:
Dne 21.1.2019 v 13:40 Mark Brown napsal(a):
It was the bit about adding more extended permission control that I was worried about there, not the initial O_APPEND bit. Indeed the O_APPEND bit sounds like it might also work from the base buffer sharing point of view, I have to confess I'd not heard of that feature before (it didn't come up in the discussion when Eric raised this in Prague).
With permissions, I meant to make possible to restrict the file descriptor operations (ioctls) for the depending task (like access to the DMA buffer, synchronize it for the non-coherent platforms and maybe read/write the actual position, delay etc.). It should be relatively easy to implement using the snd_pcm_file structure.
Right, that's what I understood you to mean. If you want to have a policy saying "it's OK to export a PCM file descriptor if it's only got permissions X and Y" the security module is going to need to know about the mechanism for setting those permissions. With dma_buf that's all a bit easier as there's less new stuff, though I've no real idea how much of a big deal that actually is.
There are many ways to implement such a thing, yeah. If we'd need an implementation that is done solely in the sound driver layer, I can imagine to introduce either a new ioctl or an open flag (like O_EXCL) to specify the restricted sharing. That is, a kind of master / slave model where only the master is allowed to manipulate the stream while the slave can mmap, read/write and get status.
In order to support EXCLUSIVE mode, it is necessary to convert the /dev/snd/ descriptor to an anon_inode:dmabuffer file descriptor. SELinux allows that file descriptor to be passed to the client. It can also be used by the AAudioService.
Okay, so this is probably the only point which we should resolve for the already available DMA buffer sharing in ALSA (the O_APPEND flag).
I had another glance to your dma-buf implementation and I see many things which might cause problems:
- allow to call dma-buf ioctls only when the audio device is in specific state (stream is not running)
- as Takashi mentioned, if we return another file-descriptor (dma-buf export) to the user space and the server closes the main pcm file-descriptor (the client does not) - the result will be a crash (dma buffer will be freed, but referenced through the dma-buf interface)
- the attach function calls dma_buf_get(fd), but what if fd points to another dma-buf allocation from a different driver? the unexpected private data will cause crash - there should be a type checking in the dma-buf interface
If I look to the dma_buf_fd() implementation:
fd = get_unused_fd_flags(flags); fd_install(fd, dmabuf->file);
.. what if we just add one new ioctl to the ALSA's PCM API which will return a new anonymous inode descriptor with the restricted access to the main PCM device to satisfy the SELinux requirements / security policies? It might be more nice and simple solution than to implement the full dma-buf interface for the ALSA's PCM devices.
Question: The dma-buf also implements the fencing, but I am not able to determine, if this mechanism is used in android [1]. It may allow concurrent mmap and synchronize apps - but the sound server should manage the access to the DMA buffer anyway. In my opinion, it makes much sense for the video-pipes when the hardware does some accelerations (encoding/decoding).
Jaroslav
On Thu, Jan 24, 2019 at 02:43:02PM +0100, Jaroslav Kysela wrote:
If I look to the dma_buf_fd() implementation:
fd = get_unused_fd_flags(flags); fd_install(fd, dmabuf->file);
.. what if we just add one new ioctl to the ALSA's PCM API which will return a new anonymous inode descriptor with the restricted access to the main PCM device to satisfy the SELinux requirements / security policies? It might be more nice and simple solution than to implement the full dma-buf interface for the ALSA's PCM devices.
That certainly works for me so long as the security people are happy.
Question: The dma-buf also implements the fencing, but I am not able to determine, if this mechanism is used in android [1]. It may allow concurrent mmap and synchronize apps - but the sound server should manage the access to the DMA buffer anyway. In my opinion, it makes much sense for the video-pipes when the hardware does some accelerations (encoding/decoding).
We had the same discuission off list and couldn't think of a need for that feature in the audio context but left it in as it's already there with dma-buf so there's no real cost to implementing it and we weren't sure we weren't missing something.
Hi Jaroslav, On Thu, 24 Jan 2019 at 21:43, Jaroslav Kysela perex@perex.cz wrote:
Dne 23.1.2019 v 13:46 Leo Yan napsal(a):
Hi all,
On Wed, Jan 23, 2019 at 12:58:51PM +0100, Takashi Iwai wrote:
On Tue, 22 Jan 2019 21:25:35 +0100, Mark Brown wrote:
On Mon, Jan 21, 2019 at 03:15:43PM +0100, Jaroslav Kysela wrote:
Dne 21.1.2019 v 13:40 Mark Brown napsal(a):
It was the bit about adding more extended permission control that I was worried about there, not the initial O_APPEND bit. Indeed the O_APPEND bit sounds like it might also work from the base buffer sharing point of view, I have to confess I'd not heard of that feature before (it didn't come up in the discussion when Eric raised this in Prague).
With permissions, I meant to make possible to restrict the file descriptor operations (ioctls) for the depending task (like access to the DMA buffer, synchronize it for the non-coherent platforms and maybe read/write the actual position, delay etc.). It should be relatively easy to implement using the snd_pcm_file structure.
Right, that's what I understood you to mean. If you want to have a policy saying "it's OK to export a PCM file descriptor if it's only got permissions X and Y" the security module is going to need to know about the mechanism for setting those permissions. With dma_buf that's all a bit easier as there's less new stuff, though I've no real idea how much of a big deal that actually is.
There are many ways to implement such a thing, yeah. If we'd need an implementation that is done solely in the sound driver layer, I can imagine to introduce either a new ioctl or an open flag (like O_EXCL) to specify the restricted sharing. That is, a kind of master / slave model where only the master is allowed to manipulate the stream while the slave can mmap, read/write and get status.
In order to support EXCLUSIVE mode, it is necessary to convert the /dev/snd/ descriptor to an anon_inode:dmabuffer file descriptor. SELinux allows that file descriptor to be passed to the client. It can also be used by the AAudioService.
Okay, so this is probably the only point which we should resolve for the already available DMA buffer sharing in ALSA (the O_APPEND flag).
I had another glance to your dma-buf implementation and I see many things which might cause problems:
- allow to call dma-buf ioctls only when the audio device is in specific
state (stream is not running)
Right. Will fix.
- as Takashi mentioned, if we return another file-descriptor (dma-buf
export) to the user space and the server closes the main pcm file-descriptor (the client does not) - the result will be a crash (dma buffer will be freed, but referenced through the dma-buf interface)
Yes, will fix.
- the attach function calls dma_buf_get(fd), but what if fd points to
another dma-buf allocation from a different driver? the unexpected private data will cause crash - there should be a type checking in the dma-buf interface
There is a validation (is_dma_buf_file() ) in dma_buf_get() function before getting the dma buffer.
If I look to the dma_buf_fd() implementation:
fd = get_unused_fd_flags(flags); fd_install(fd, dmabuf->file);
.. what if we just add one new ioctl to the ALSA's PCM API which will return a new anonymous inode descriptor with the restricted access to the main PCM device to satisfy the SELinux requirements / security policies? It might be more nice and simple solution than to implement the full dma-buf interface for the ALSA's PCM devices.
I will do some investigation for your suggestion and talk with the security people if it can work. Thanks for your suggestion.
On Fri, 25 Jan 2019 10:25:37 +0100, Baolin Wang wrote:
Hi Jaroslav, On Thu, 24 Jan 2019 at 21:43, Jaroslav Kysela perex@perex.cz wrote:
Dne 23.1.2019 v 13:46 Leo Yan napsal(a):
Hi all,
On Wed, Jan 23, 2019 at 12:58:51PM +0100, Takashi Iwai wrote:
On Tue, 22 Jan 2019 21:25:35 +0100, Mark Brown wrote:
On Mon, Jan 21, 2019 at 03:15:43PM +0100, Jaroslav Kysela wrote:
Dne 21.1.2019 v 13:40 Mark Brown napsal(a):
> It was the bit about adding more extended permission control that I was > worried about there, not the initial O_APPEND bit. Indeed the O_APPEND > bit sounds like it might also work from the base buffer sharing point of > view, I have to confess I'd not heard of that feature before (it didn't > come up in the discussion when Eric raised this in Prague).
With permissions, I meant to make possible to restrict the file descriptor operations (ioctls) for the depending task (like access to the DMA buffer, synchronize it for the non-coherent platforms and maybe read/write the actual position, delay etc.). It should be relatively easy to implement using the snd_pcm_file structure.
Right, that's what I understood you to mean. If you want to have a policy saying "it's OK to export a PCM file descriptor if it's only got permissions X and Y" the security module is going to need to know about the mechanism for setting those permissions. With dma_buf that's all a bit easier as there's less new stuff, though I've no real idea how much of a big deal that actually is.
There are many ways to implement such a thing, yeah. If we'd need an implementation that is done solely in the sound driver layer, I can imagine to introduce either a new ioctl or an open flag (like O_EXCL) to specify the restricted sharing. That is, a kind of master / slave model where only the master is allowed to manipulate the stream while the slave can mmap, read/write and get status.
In order to support EXCLUSIVE mode, it is necessary to convert the /dev/snd/ descriptor to an anon_inode:dmabuffer file descriptor. SELinux allows that file descriptor to be passed to the client. It can also be used by the AAudioService.
Okay, so this is probably the only point which we should resolve for the already available DMA buffer sharing in ALSA (the O_APPEND flag).
I had another glance to your dma-buf implementation and I see many things which might cause problems:
- allow to call dma-buf ioctls only when the audio device is in specific
state (stream is not running)
Right. Will fix.
- as Takashi mentioned, if we return another file-descriptor (dma-buf
export) to the user space and the server closes the main pcm file-descriptor (the client does not) - the result will be a crash (dma buffer will be freed, but referenced through the dma-buf interface)
Yes, will fix.
There are a few more overlooked problems. A part of them was already mentioned in my previous reply, but let me repeat:
- The racy ioctls have to be considered; you can perform this export ioctl concurrently, and both of them write and mix up the setup, which is obviously broken.
- The PCM buffer can be re-allocated on the fly. If the current buffer is abandoned while exporting, it leads to the UAF.
- Similarly, what if the PCM stream that is attached is closed without detaching itself? Or, what if the PCM stream attaches itself twice without detaching?
- The driver may provide its own mmap method, and you can't hard-code the mmap implementation as currently in snd_pcm_dmabuf_mmap().
I suppose you can drop of most of the code in snd_pcm_dmabuf_map(), instead, assign PCM substream in obj, and call snd_pcm_mmap_data() with the given VMA. If this really works, it manages the mmap refcount, so the previous two issues should be covered there. But it needs more consideration...
- What happens to the PCM buffer that has been allocated before attaching, if it's not the pre-allocated one? It should be released properly beforehand, otherwise leaks.
- There is no validation of the attached dma-buf pages; most drivers set coherent DMA mask, and they rely on it. e.g. if a page over the DMA mask is passed, it will break silently.
- Some drivers don't use the standard memory pages but keep their own hardware buffer (e.g. rme96 or rm32 driver). This ioctl would be completely broken on such hardware. That is, we need some sanity check whether the PCM allows the arbitrary dma-buf or not.
thanks,
Takashi
On Fri, 25 Jan 2019 11:10:25 +0100, Takashi Iwai wrote:
On Fri, 25 Jan 2019 10:25:37 +0100, Baolin Wang wrote:
Hi Jaroslav, On Thu, 24 Jan 2019 at 21:43, Jaroslav Kysela perex@perex.cz wrote:
Dne 23.1.2019 v 13:46 Leo Yan napsal(a):
Hi all,
On Wed, Jan 23, 2019 at 12:58:51PM +0100, Takashi Iwai wrote:
On Tue, 22 Jan 2019 21:25:35 +0100, Mark Brown wrote:
On Mon, Jan 21, 2019 at 03:15:43PM +0100, Jaroslav Kysela wrote: > Dne 21.1.2019 v 13:40 Mark Brown napsal(a):
>> It was the bit about adding more extended permission control that I was >> worried about there, not the initial O_APPEND bit. Indeed the O_APPEND >> bit sounds like it might also work from the base buffer sharing point of >> view, I have to confess I'd not heard of that feature before (it didn't >> come up in the discussion when Eric raised this in Prague).
> With permissions, I meant to make possible to restrict the file > descriptor operations (ioctls) for the depending task (like access to > the DMA buffer, synchronize it for the non-coherent platforms and maybe > read/write the actual position, delay etc.). It should be relatively > easy to implement using the snd_pcm_file structure.
Right, that's what I understood you to mean. If you want to have a policy saying "it's OK to export a PCM file descriptor if it's only got permissions X and Y" the security module is going to need to know about the mechanism for setting those permissions. With dma_buf that's all a bit easier as there's less new stuff, though I've no real idea how much of a big deal that actually is.
There are many ways to implement such a thing, yeah. If we'd need an implementation that is done solely in the sound driver layer, I can imagine to introduce either a new ioctl or an open flag (like O_EXCL) to specify the restricted sharing. That is, a kind of master / slave model where only the master is allowed to manipulate the stream while the slave can mmap, read/write and get status.
In order to support EXCLUSIVE mode, it is necessary to convert the /dev/snd/ descriptor to an anon_inode:dmabuffer file descriptor. SELinux allows that file descriptor to be passed to the client. It can also be used by the AAudioService.
Okay, so this is probably the only point which we should resolve for the already available DMA buffer sharing in ALSA (the O_APPEND flag).
I had another glance to your dma-buf implementation and I see many things which might cause problems:
- allow to call dma-buf ioctls only when the audio device is in specific
state (stream is not running)
Right. Will fix.
- as Takashi mentioned, if we return another file-descriptor (dma-buf
export) to the user space and the server closes the main pcm file-descriptor (the client does not) - the result will be a crash (dma buffer will be freed, but referenced through the dma-buf interface)
Yes, will fix.
There are a few more overlooked problems. A part of them was already mentioned in my previous reply, but let me repeat:
The racy ioctls have to be considered; you can perform this export ioctl concurrently, and both of them write and mix up the setup, which is obviously broken.
The PCM buffer can be re-allocated on the fly. If the current buffer is abandoned while exporting, it leads to the UAF.
Similarly, what if the PCM stream that is attached is closed without detaching itself? Or, what if the PCM stream attaches itself twice without detaching?
The driver may provide its own mmap method, and you can't hard-code the mmap implementation as currently in snd_pcm_dmabuf_mmap().
I suppose you can drop of most of the code in snd_pcm_dmabuf_map(), instead, assign PCM substream in obj, and call snd_pcm_mmap_data() with the given VMA. If this really works, it manages the mmap refcount, so the previous two issues should be covered there. But it needs more consideration...
Erm, obviously it's not enough. Each attach / detach needs to manage the refcount, too, for covering the cases above. It can re-use the PCM mmap_refount, though.
Takashi
On Fri, 25 Jan 2019 at 18:20, Takashi Iwai tiwai@suse.de wrote:
On Fri, 25 Jan 2019 11:10:25 +0100, Takashi Iwai wrote:
On Fri, 25 Jan 2019 10:25:37 +0100, Baolin Wang wrote:
Hi Jaroslav, On Thu, 24 Jan 2019 at 21:43, Jaroslav Kysela perex@perex.cz wrote:
Dne 23.1.2019 v 13:46 Leo Yan napsal(a):
Hi all,
On Wed, Jan 23, 2019 at 12:58:51PM +0100, Takashi Iwai wrote:
On Tue, 22 Jan 2019 21:25:35 +0100, Mark Brown wrote: > > On Mon, Jan 21, 2019 at 03:15:43PM +0100, Jaroslav Kysela wrote: >> Dne 21.1.2019 v 13:40 Mark Brown napsal(a): > >>> It was the bit about adding more extended permission control that I was >>> worried about there, not the initial O_APPEND bit. Indeed the O_APPEND >>> bit sounds like it might also work from the base buffer sharing point of >>> view, I have to confess I'd not heard of that feature before (it didn't >>> come up in the discussion when Eric raised this in Prague). > >> With permissions, I meant to make possible to restrict the file >> descriptor operations (ioctls) for the depending task (like access to >> the DMA buffer, synchronize it for the non-coherent platforms and maybe >> read/write the actual position, delay etc.). It should be relatively >> easy to implement using the snd_pcm_file structure. > > Right, that's what I understood you to mean. If you want to have a > policy saying "it's OK to export a PCM file descriptor if it's only got > permissions X and Y" the security module is going to need to know about > the mechanism for setting those permissions. With dma_buf that's all a > bit easier as there's less new stuff, though I've no real idea how much > of a big deal that actually is.
There are many ways to implement such a thing, yeah. If we'd need an implementation that is done solely in the sound driver layer, I can imagine to introduce either a new ioctl or an open flag (like O_EXCL) to specify the restricted sharing. That is, a kind of master / slave model where only the master is allowed to manipulate the stream while the slave can mmap, read/write and get status.
In order to support EXCLUSIVE mode, it is necessary to convert the /dev/snd/ descriptor to an anon_inode:dmabuffer file descriptor. SELinux allows that file descriptor to be passed to the client. It can also be used by the AAudioService.
Okay, so this is probably the only point which we should resolve for the already available DMA buffer sharing in ALSA (the O_APPEND flag).
I had another glance to your dma-buf implementation and I see many things which might cause problems:
- allow to call dma-buf ioctls only when the audio device is in specific
state (stream is not running)
Right. Will fix.
- as Takashi mentioned, if we return another file-descriptor (dma-buf
export) to the user space and the server closes the main pcm file-descriptor (the client does not) - the result will be a crash (dma buffer will be freed, but referenced through the dma-buf interface)
Yes, will fix.
There are a few more overlooked problems. A part of them was already mentioned in my previous reply, but let me repeat:
The racy ioctls have to be considered; you can perform this export ioctl concurrently, and both of them write and mix up the setup, which is obviously broken.
The PCM buffer can be re-allocated on the fly. If the current buffer is abandoned while exporting, it leads to the UAF.
Similarly, what if the PCM stream that is attached is closed without detaching itself? Or, what if the PCM stream attaches itself twice without detaching?
The driver may provide its own mmap method, and you can't hard-code the mmap implementation as currently in snd_pcm_dmabuf_mmap().
I suppose you can drop of most of the code in snd_pcm_dmabuf_map(), instead, assign PCM substream in obj, and call snd_pcm_mmap_data() with the given VMA. If this really works, it manages the mmap refcount, so the previous two issues should be covered there. But it needs more consideration...
Erm, obviously it's not enough. Each attach / detach needs to manage the refcount, too, for covering the cases above. It can re-use the PCM mmap_refount, though.
But we've used the DMA buffer file's refcounting to manage the DMA buffer. So is this not enough?
On Fri, 25 Jan 2019 12:24:29 +0100, Baolin Wang wrote:
On Fri, 25 Jan 2019 at 18:20, Takashi Iwai tiwai@suse.de wrote:
On Fri, 25 Jan 2019 11:10:25 +0100, Takashi Iwai wrote:
On Fri, 25 Jan 2019 10:25:37 +0100, Baolin Wang wrote:
Hi Jaroslav, On Thu, 24 Jan 2019 at 21:43, Jaroslav Kysela perex@perex.cz wrote:
Dne 23.1.2019 v 13:46 Leo Yan napsal(a):
Hi all,
On Wed, Jan 23, 2019 at 12:58:51PM +0100, Takashi Iwai wrote: > On Tue, 22 Jan 2019 21:25:35 +0100, > Mark Brown wrote: >> >> On Mon, Jan 21, 2019 at 03:15:43PM +0100, Jaroslav Kysela wrote: >>> Dne 21.1.2019 v 13:40 Mark Brown napsal(a): >> >>>> It was the bit about adding more extended permission control that I was >>>> worried about there, not the initial O_APPEND bit. Indeed the O_APPEND >>>> bit sounds like it might also work from the base buffer sharing point of >>>> view, I have to confess I'd not heard of that feature before (it didn't >>>> come up in the discussion when Eric raised this in Prague). >> >>> With permissions, I meant to make possible to restrict the file >>> descriptor operations (ioctls) for the depending task (like access to >>> the DMA buffer, synchronize it for the non-coherent platforms and maybe >>> read/write the actual position, delay etc.). It should be relatively >>> easy to implement using the snd_pcm_file structure. >> >> Right, that's what I understood you to mean. If you want to have a >> policy saying "it's OK to export a PCM file descriptor if it's only got >> permissions X and Y" the security module is going to need to know about >> the mechanism for setting those permissions. With dma_buf that's all a >> bit easier as there's less new stuff, though I've no real idea how much >> of a big deal that actually is. > > There are many ways to implement such a thing, yeah. If we'd need an > implementation that is done solely in the sound driver layer, I can > imagine to introduce either a new ioctl or an open flag (like O_EXCL) > to specify the restricted sharing. That is, a kind of master / slave > model where only the master is allowed to manipulate the stream while > the slave can mmap, read/write and get status.
In order to support EXCLUSIVE mode, it is necessary to convert the /dev/snd/ descriptor to an anon_inode:dmabuffer file descriptor. SELinux allows that file descriptor to be passed to the client. It can also be used by the AAudioService.
Okay, so this is probably the only point which we should resolve for the already available DMA buffer sharing in ALSA (the O_APPEND flag).
I had another glance to your dma-buf implementation and I see many things which might cause problems:
- allow to call dma-buf ioctls only when the audio device is in specific
state (stream is not running)
Right. Will fix.
- as Takashi mentioned, if we return another file-descriptor (dma-buf
export) to the user space and the server closes the main pcm file-descriptor (the client does not) - the result will be a crash (dma buffer will be freed, but referenced through the dma-buf interface)
Yes, will fix.
There are a few more overlooked problems. A part of them was already mentioned in my previous reply, but let me repeat:
The racy ioctls have to be considered; you can perform this export ioctl concurrently, and both of them write and mix up the setup, which is obviously broken.
The PCM buffer can be re-allocated on the fly. If the current buffer is abandoned while exporting, it leads to the UAF.
Similarly, what if the PCM stream that is attached is closed without detaching itself? Or, what if the PCM stream attaches itself twice without detaching?
The driver may provide its own mmap method, and you can't hard-code the mmap implementation as currently in snd_pcm_dmabuf_mmap().
I suppose you can drop of most of the code in snd_pcm_dmabuf_map(), instead, assign PCM substream in obj, and call snd_pcm_mmap_data() with the given VMA. If this really works, it manages the mmap refcount, so the previous two issues should be covered there. But it needs more consideration...
Erm, obviously it's not enough. Each attach / detach needs to manage the refcount, too, for covering the cases above. It can re-use the PCM mmap_refount, though.
But we've used the DMA buffer file's refcounting to manage the DMA buffer. So is this not enough?
Unless you manage the PCM substream refcount (or block the state change), the PCM stream itself can be released (or re-setup) freely.
Takashi
On Fri, 25 Jan 2019 at 21:04, Takashi Iwai tiwai@suse.de wrote:
Erm, obviously it's not enough. Each attach / detach needs to manage the refcount, too, for covering the cases above. It can re-use the PCM mmap_refount, though.
But we've used the DMA buffer file's refcounting to manage the DMA buffer. So is this not enough?
Unless you manage the PCM substream refcount (or block the state change), the PCM stream itself can be released (or re-setup) freely.
OK. Thanks for your comments.
Hi Takashi, On Fri, 25 Jan 2019 at 18:10, Takashi Iwai tiwai@suse.de wrote:
On Fri, 25 Jan 2019 10:25:37 +0100, Baolin Wang wrote:
Hi Jaroslav, On Thu, 24 Jan 2019 at 21:43, Jaroslav Kysela perex@perex.cz wrote:
Dne 23.1.2019 v 13:46 Leo Yan napsal(a):
Hi all,
On Wed, Jan 23, 2019 at 12:58:51PM +0100, Takashi Iwai wrote:
On Tue, 22 Jan 2019 21:25:35 +0100, Mark Brown wrote:
On Mon, Jan 21, 2019 at 03:15:43PM +0100, Jaroslav Kysela wrote: > Dne 21.1.2019 v 13:40 Mark Brown napsal(a):
>> It was the bit about adding more extended permission control that I was >> worried about there, not the initial O_APPEND bit. Indeed the O_APPEND >> bit sounds like it might also work from the base buffer sharing point of >> view, I have to confess I'd not heard of that feature before (it didn't >> come up in the discussion when Eric raised this in Prague).
> With permissions, I meant to make possible to restrict the file > descriptor operations (ioctls) for the depending task (like access to > the DMA buffer, synchronize it for the non-coherent platforms and maybe > read/write the actual position, delay etc.). It should be relatively > easy to implement using the snd_pcm_file structure.
Right, that's what I understood you to mean. If you want to have a policy saying "it's OK to export a PCM file descriptor if it's only got permissions X and Y" the security module is going to need to know about the mechanism for setting those permissions. With dma_buf that's all a bit easier as there's less new stuff, though I've no real idea how much of a big deal that actually is.
There are many ways to implement such a thing, yeah. If we'd need an implementation that is done solely in the sound driver layer, I can imagine to introduce either a new ioctl or an open flag (like O_EXCL) to specify the restricted sharing. That is, a kind of master / slave model where only the master is allowed to manipulate the stream while the slave can mmap, read/write and get status.
In order to support EXCLUSIVE mode, it is necessary to convert the /dev/snd/ descriptor to an anon_inode:dmabuffer file descriptor. SELinux allows that file descriptor to be passed to the client. It can also be used by the AAudioService.
Okay, so this is probably the only point which we should resolve for the already available DMA buffer sharing in ALSA (the O_APPEND flag).
I had another glance to your dma-buf implementation and I see many things which might cause problems:
- allow to call dma-buf ioctls only when the audio device is in specific
state (stream is not running)
Right. Will fix.
- as Takashi mentioned, if we return another file-descriptor (dma-buf
export) to the user space and the server closes the main pcm file-descriptor (the client does not) - the result will be a crash (dma buffer will be freed, but referenced through the dma-buf interface)
Yes, will fix.
There are a few more overlooked problems. A part of them was already mentioned in my previous reply, but let me repeat:
- The racy ioctls have to be considered; you can perform this export ioctl concurrently, and both of them write and mix up the setup, which is obviously broken.
Yes, I think I missed the snd_pcm_stream_lock, and will add.
- The PCM buffer can be re-allocated on the fly. If the current buffer is abandoned while exporting, it leads to the UAF.
So I need some validation to check if the buffer is available now when exporting it.
- Similarly, what if the PCM stream that is attached is closed without detaching itself? Or, what if the PCM stream attaches itself twice
We will detach it automatically in snd_pcm_free_stream()
without detaching?
Sure, need add validation for this case.
The driver may provide its own mmap method, and you can't hard-code the mmap implementation as currently in snd_pcm_dmabuf_mmap().
I suppose you can drop of most of the code in snd_pcm_dmabuf_map(), instead, assign PCM substream in obj, and call snd_pcm_mmap_data() with the given VMA. If this really works, it manages the mmap refcount, so the previous two issues should be covered there. But it needs more consideration...
Ah, I think I missed the snd_pcm_mmap_data() function. Yes, so I can remove the implementation in snd_pcm_dmabuf_map().
- What happens to the PCM buffer that has been allocated before attaching, if it's not the pre-allocated one? It should be released properly beforehand, otherwise leaks.
I am not sure I understood you correctly. If the PCM buffer has been allocated, the platform driver should handle it? Since we always use substream->dma_buffer.
- There is no validation of the attached dma-buf pages; most drivers set coherent DMA mask, and they rely on it. e.g. if a page over the DMA mask is passed, it will break silently.
Sorry maybe I did not get your point here. We have validate the dma_map_sg_attr() funtion, in this fucntion it will validate the DMA mask by dma_capable().
- Some drivers don't use the standard memory pages but keep their own hardware buffer (e.g. rme96 or rm32 driver). This ioctl would be completely broken on such hardware. That is, we need some sanity check whether the PCM allows the arbitrary dma-buf or not.
Make sense. Need add some validation to make sure if this PCM can export or not.
Very appreciated for your useful comments.
On Fri, 25 Jan 2019 12:11:43 +0100, Baolin Wang wrote:
Hi Takashi, On Fri, 25 Jan 2019 at 18:10, Takashi Iwai tiwai@suse.de wrote:
On Fri, 25 Jan 2019 10:25:37 +0100, Baolin Wang wrote:
Hi Jaroslav, On Thu, 24 Jan 2019 at 21:43, Jaroslav Kysela perex@perex.cz wrote:
Dne 23.1.2019 v 13:46 Leo Yan napsal(a):
Hi all,
On Wed, Jan 23, 2019 at 12:58:51PM +0100, Takashi Iwai wrote:
On Tue, 22 Jan 2019 21:25:35 +0100, Mark Brown wrote: > > On Mon, Jan 21, 2019 at 03:15:43PM +0100, Jaroslav Kysela wrote: >> Dne 21.1.2019 v 13:40 Mark Brown napsal(a): > >>> It was the bit about adding more extended permission control that I was >>> worried about there, not the initial O_APPEND bit. Indeed the O_APPEND >>> bit sounds like it might also work from the base buffer sharing point of >>> view, I have to confess I'd not heard of that feature before (it didn't >>> come up in the discussion when Eric raised this in Prague). > >> With permissions, I meant to make possible to restrict the file >> descriptor operations (ioctls) for the depending task (like access to >> the DMA buffer, synchronize it for the non-coherent platforms and maybe >> read/write the actual position, delay etc.). It should be relatively >> easy to implement using the snd_pcm_file structure. > > Right, that's what I understood you to mean. If you want to have a > policy saying "it's OK to export a PCM file descriptor if it's only got > permissions X and Y" the security module is going to need to know about > the mechanism for setting those permissions. With dma_buf that's all a > bit easier as there's less new stuff, though I've no real idea how much > of a big deal that actually is.
There are many ways to implement such a thing, yeah. If we'd need an implementation that is done solely in the sound driver layer, I can imagine to introduce either a new ioctl or an open flag (like O_EXCL) to specify the restricted sharing. That is, a kind of master / slave model where only the master is allowed to manipulate the stream while the slave can mmap, read/write and get status.
In order to support EXCLUSIVE mode, it is necessary to convert the /dev/snd/ descriptor to an anon_inode:dmabuffer file descriptor. SELinux allows that file descriptor to be passed to the client. It can also be used by the AAudioService.
Okay, so this is probably the only point which we should resolve for the already available DMA buffer sharing in ALSA (the O_APPEND flag).
I had another glance to your dma-buf implementation and I see many things which might cause problems:
- allow to call dma-buf ioctls only when the audio device is in specific
state (stream is not running)
Right. Will fix.
- as Takashi mentioned, if we return another file-descriptor (dma-buf
export) to the user space and the server closes the main pcm file-descriptor (the client does not) - the result will be a crash (dma buffer will be freed, but referenced through the dma-buf interface)
Yes, will fix.
There are a few more overlooked problems. A part of them was already mentioned in my previous reply, but let me repeat:
- The racy ioctls have to be considered; you can perform this export ioctl concurrently, and both of them write and mix up the setup, which is obviously broken.
Yes, I think I missed the snd_pcm_stream_lock, and will add.
Beware that it's not so trivial. The stream lock is usually spinlock.
In addition, we need to be careful about the PCM state, as Jaroslav mentioned. Basically SNDRV_PCM_STATE_SETUP is already too late for attaching the buffer, since another buffer is already assigned to the stream. Similarly, after detaching, the stream state must go to SNDRV_PCM_STATE_OPEN.
- What happens to the PCM buffer that has been allocated before attaching, if it's not the pre-allocated one? It should be released properly beforehand, otherwise leaks.
I am not sure I understood you correctly. If the PCM buffer has been allocated, the platform driver should handle it? Since we always use substream->dma_buffer.
substream->dma_buffer is merely a pre-allocated buffer, and not every driver sets it up. The actual PCM buffer is found in substream's runtime, and this implementation isn't always with the preallocated buffer. It can even be a fixed IO-mapped buffer.
- There is no validation of the attached dma-buf pages; most drivers set coherent DMA mask, and they rely on it. e.g. if a page over the DMA mask is passed, it will break silently.
Sorry maybe I did not get your point here. We have validate the dma_map_sg_attr() funtion, in this fucntion it will validate the DMA mask by dma_capable().
OK, then this should be fine -- at least about DMA mask. But, how about other setups, e.g. coherency?
Imagine that a buffer allocated for chip A is exported to another chip B. If chip B requires some special setup of the pages while A isn't, this won't work. For example, some HD-audio chips require the non-cached pages while some HD-audio chips allow normal pages.
thanks,
Takashi
On Fri, 25 Jan 2019 at 21:03, Takashi Iwai tiwai@suse.de wrote:
On Fri, 25 Jan 2019 12:11:43 +0100, Baolin Wang wrote:
Hi Takashi, On Fri, 25 Jan 2019 at 18:10, Takashi Iwai tiwai@suse.de wrote:
On Fri, 25 Jan 2019 10:25:37 +0100, Baolin Wang wrote:
Hi Jaroslav, On Thu, 24 Jan 2019 at 21:43, Jaroslav Kysela perex@perex.cz wrote:
Dne 23.1.2019 v 13:46 Leo Yan napsal(a):
Hi all,
On Wed, Jan 23, 2019 at 12:58:51PM +0100, Takashi Iwai wrote: > On Tue, 22 Jan 2019 21:25:35 +0100, > Mark Brown wrote: >> >> On Mon, Jan 21, 2019 at 03:15:43PM +0100, Jaroslav Kysela wrote: >>> Dne 21.1.2019 v 13:40 Mark Brown napsal(a): >> >>>> It was the bit about adding more extended permission control that I was >>>> worried about there, not the initial O_APPEND bit. Indeed the O_APPEND >>>> bit sounds like it might also work from the base buffer sharing point of >>>> view, I have to confess I'd not heard of that feature before (it didn't >>>> come up in the discussion when Eric raised this in Prague). >> >>> With permissions, I meant to make possible to restrict the file >>> descriptor operations (ioctls) for the depending task (like access to >>> the DMA buffer, synchronize it for the non-coherent platforms and maybe >>> read/write the actual position, delay etc.). It should be relatively >>> easy to implement using the snd_pcm_file structure. >> >> Right, that's what I understood you to mean. If you want to have a >> policy saying "it's OK to export a PCM file descriptor if it's only got >> permissions X and Y" the security module is going to need to know about >> the mechanism for setting those permissions. With dma_buf that's all a >> bit easier as there's less new stuff, though I've no real idea how much >> of a big deal that actually is. > > There are many ways to implement such a thing, yeah. If we'd need an > implementation that is done solely in the sound driver layer, I can > imagine to introduce either a new ioctl or an open flag (like O_EXCL) > to specify the restricted sharing. That is, a kind of master / slave > model where only the master is allowed to manipulate the stream while > the slave can mmap, read/write and get status.
In order to support EXCLUSIVE mode, it is necessary to convert the /dev/snd/ descriptor to an anon_inode:dmabuffer file descriptor. SELinux allows that file descriptor to be passed to the client. It can also be used by the AAudioService.
Okay, so this is probably the only point which we should resolve for the already available DMA buffer sharing in ALSA (the O_APPEND flag).
I had another glance to your dma-buf implementation and I see many things which might cause problems:
- allow to call dma-buf ioctls only when the audio device is in specific
state (stream is not running)
Right. Will fix.
- as Takashi mentioned, if we return another file-descriptor (dma-buf
export) to the user space and the server closes the main pcm file-descriptor (the client does not) - the result will be a crash (dma buffer will be freed, but referenced through the dma-buf interface)
Yes, will fix.
There are a few more overlooked problems. A part of them was already mentioned in my previous reply, but let me repeat:
- The racy ioctls have to be considered; you can perform this export ioctl concurrently, and both of them write and mix up the setup, which is obviously broken.
Yes, I think I missed the snd_pcm_stream_lock, and will add.
Beware that it's not so trivial. The stream lock is usually spinlock.
Right. Thanks for your reminding.
In addition, we need to be careful about the PCM state, as Jaroslav mentioned. Basically SNDRV_PCM_STATE_SETUP is already too late for attaching the buffer, since another buffer is already assigned to the stream. Similarly, after detaching, the stream state must go to SNDRV_PCM_STATE_OPEN.
Make sense.
- What happens to the PCM buffer that has been allocated before attaching, if it's not the pre-allocated one? It should be released properly beforehand, otherwise leaks.
I am not sure I understood you correctly. If the PCM buffer has been allocated, the platform driver should handle it? Since we always use substream->dma_buffer.
substream->dma_buffer is merely a pre-allocated buffer, and not every driver sets it up. The actual PCM buffer is found in substream's runtime, and this implementation isn't always with the preallocated buffer. It can even be a fixed IO-mapped buffer.
OK. Thanks for your explanation.
- There is no validation of the attached dma-buf pages; most drivers set coherent DMA mask, and they rely on it. e.g. if a page over the DMA mask is passed, it will break silently.
Sorry maybe I did not get your point here. We have validate the dma_map_sg_attr() funtion, in this fucntion it will validate the DMA mask by dma_capable().
OK, then this should be fine -- at least about DMA mask. But, how about other setups, e.g. coherency?
Imagine that a buffer allocated for chip A is exported to another chip B. If chip B requires some special setup of the pages while A isn't, this won't work. For example, some HD-audio chips require the non-cached pages while some HD-audio chips allow normal pages.
OK. That's one case need to validate. Thanks for your comments.
On Wed, 23 Jan 2019 13:46:58 +0100, Leo Yan wrote:
Hi all,
On Wed, Jan 23, 2019 at 12:58:51PM +0100, Takashi Iwai wrote:
On Tue, 22 Jan 2019 21:25:35 +0100, Mark Brown wrote:
On Mon, Jan 21, 2019 at 03:15:43PM +0100, Jaroslav Kysela wrote:
Dne 21.1.2019 v 13:40 Mark Brown napsal(a):
It was the bit about adding more extended permission control that I was worried about there, not the initial O_APPEND bit. Indeed the O_APPEND bit sounds like it might also work from the base buffer sharing point of view, I have to confess I'd not heard of that feature before (it didn't come up in the discussion when Eric raised this in Prague).
With permissions, I meant to make possible to restrict the file descriptor operations (ioctls) for the depending task (like access to the DMA buffer, synchronize it for the non-coherent platforms and maybe read/write the actual position, delay etc.). It should be relatively easy to implement using the snd_pcm_file structure.
Right, that's what I understood you to mean. If you want to have a policy saying "it's OK to export a PCM file descriptor if it's only got permissions X and Y" the security module is going to need to know about the mechanism for setting those permissions. With dma_buf that's all a bit easier as there's less new stuff, though I've no real idea how much of a big deal that actually is.
There are many ways to implement such a thing, yeah. If we'd need an implementation that is done solely in the sound driver layer, I can imagine to introduce either a new ioctl or an open flag (like O_EXCL) to specify the restricted sharing. That is, a kind of master / slave model where only the master is allowed to manipulate the stream while the slave can mmap, read/write and get status.
I am lacking security related knowledge, especially for SELinux. So only can give background information but not sure if it's really helpful for discussion.
Android web page [1] give some information for this:
"The shared memory is referenced using a file descriptor that is generated by the ALSA driver. If the file descriptor is directly associated with a /dev/snd/ driver file, then it can be used by the AAudio service in SHARED mode. But the descriptor cannot be passed to the client code for EXCLUSIVE mode. The /dev/snd/ file descriptor would provide too broad of access to the client, so it is blocked by SELinux.
In order to support EXCLUSIVE mode, it is necessary to convert the /dev/snd/ descriptor to an anon_inode:dmabuffer file descriptor. SELinux allows that file descriptor to be passed to the client. It can also be used by the AAudioService.
An anon_inode:dmabuffer file descriptor can be generated using the Android Ion memory library."
So we work out dmabuf driver for audio buffer, the audio buffer will be exported and attached by using dma-buf framework; then we can return one file descriptor which is generated by dma-buf and this file descriptor is bound with anon inode based on dma-buf core code.
If we directly use the device node /dev/snd/ as file descriptor, even though we specify flag O_EXCL when open it, but it still is not an anon inode file descriptor. Thus this is not safe enough and will be blocked by SELinux. On the other hand, this patch wants to use dma-buf framework to provide file descriptor for the audio buffer, and this audio buffer can be one of mutiple audio buffers in the system and it can be shared to any audio client program.
Again, I have no less knowledge for SELinux so sorry if I introduce any noise at here. And very appreciate any comments for this.
Hrm, it sounds like a workaround just to bypass SELinux check...
The sound server can open another PCM stream with O_APPEND, and pass that fd to the client, too?
thanks,
Takashi
On Fri, Jan 25, 2019 at 02:19:22PM +0100, Takashi Iwai wrote:
Leo Yan wrote:
If we directly use the device node /dev/snd/ as file descriptor, even though we specify flag O_EXCL when open it, but it still is not an anon inode file descriptor. Thus this is not safe enough and will be blocked by SELinux. On the other hand, this patch wants to use dma-buf framework to provide file descriptor for the audio buffer, and this audio buffer can be one of mutiple audio buffers in the system and it can be shared to any audio client program.
Hrm, it sounds like a workaround just to bypass SELinux check...
The sound server can open another PCM stream with O_APPEND, and pass that fd to the client, too?
So long as we can teach SELinux that they're safe to export, yeah.
Dne 25.1.2019 v 19:25 Mark Brown napsal(a):
On Fri, Jan 25, 2019 at 02:19:22PM +0100, Takashi Iwai wrote:
Leo Yan wrote:
If we directly use the device node /dev/snd/ as file descriptor, even though we specify flag O_EXCL when open it, but it still is not an anon inode file descriptor. Thus this is not safe enough and will be blocked by SELinux. On the other hand, this patch wants to use dma-buf framework to provide file descriptor for the audio buffer, and this audio buffer can be one of mutiple audio buffers in the system and it can be shared to any audio client program.
Hrm, it sounds like a workaround just to bypass SELinux check...
The sound server can open another PCM stream with O_APPEND, and pass that fd to the client, too?
So long as we can teach SELinux that they're safe to export, yeah.
It seems that SELinux works with the file, so the SELinux will block the fd pass, because the file descriptor (through standard dup()) continues to use the /dev/snd inode.
I would propose to implement a dup ioctl to return a new anon_inode:snd-pcm file descriptor (see bellow).
If we agree on this, I can propose the full solution.
-- Subject: [PATCH] ALSA: pcm: implement the anonymous dup (inode file descriptor)
This patch implements new SNDRV_PCM_IOCTL_ANONYMOUS_DUP ioctl which returns the new duplicated anonymous inode file descriptor (anon_inode:snd-pcm) which can be passed to the restricted clients.
This implementation is just a concept for comments - it does not contain the additional restriction control.
TODO: The clients might be restricted to disallow a set of controls (ioctls) for the audio stream.
This patch is meant to be the alternative for the dma-buf interface. Both implementation have some pros and cons:
anon_inode:dmabuf
- a bit standard export API for the DMA buffers - fencing for the concurrent access [1] - driver/kernel interface for the DMA buffer [1] - multiple attach/detach scheme [1]
[1] the real usage for the sound PCM is unknown at the moment for this feature
anon_inode:snd-pcm
- simple (no problem with ref-counting, non-standard mmap implementation etc.) - allow to use more sound interfaces for the file descriptor like status ioctls - more fine grained security policies (another anon_inode name unshared with other drivers) --- include/uapi/sound/asound.h | 1 + sound/core/pcm_compat.c | 1 + sound/core/pcm_native.c | 40 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+)
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 404d4b9ffe76..ad821a52f970 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -576,6 +576,7 @@ enum { #define SNDRV_PCM_IOCTL_TSTAMP _IOW('A', 0x02, int) #define SNDRV_PCM_IOCTL_TTSTAMP _IOW('A', 0x03, int) #define SNDRV_PCM_IOCTL_USER_PVERSION _IOW('A', 0x04, int) +#define SNDRV_PCM_IOCTL_ANONYMOUS_DUP _IOW('A', 0x05, int) #define SNDRV_PCM_IOCTL_HW_REFINE _IOWR('A', 0x10, struct snd_pcm_hw_params) #define SNDRV_PCM_IOCTL_HW_PARAMS _IOWR('A', 0x11, struct snd_pcm_hw_params) #define SNDRV_PCM_IOCTL_HW_FREE _IO('A', 0x12) diff --git a/sound/core/pcm_compat.c b/sound/core/pcm_compat.c index 946ab080ac00..22446cd574ee 100644 --- a/sound/core/pcm_compat.c +++ b/sound/core/pcm_compat.c @@ -675,6 +675,7 @@ static long snd_pcm_ioctl_compat(struct file *file, unsigned int cmd, unsigned l case SNDRV_PCM_IOCTL_TSTAMP: case SNDRV_PCM_IOCTL_TTSTAMP: case SNDRV_PCM_IOCTL_USER_PVERSION: + case SNDRV_PCM_IOCTL_ANONYMOUS_DUP: case SNDRV_PCM_IOCTL_HWSYNC: case SNDRV_PCM_IOCTL_PREPARE: case SNDRV_PCM_IOCTL_RESET: diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 26afb6b0889a..a21bb482b4b0 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -37,6 +37,8 @@ #include <sound/minors.h> #include <linux/uio.h> #include <linux/delay.h> +#include <linux/anon_inodes.h> +#include <linux/syscalls.h>
#include "pcm_local.h"
@@ -2836,6 +2838,42 @@ static int snd_pcm_forward_ioctl(struct snd_pcm_substream *substream, return result < 0 ? result : 0; }
+static int snd_pcm_anonymous_dup(struct file *file, + struct snd_pcm_substream *substream, + int __user *arg) +{ + int fd; + int res; + int dup_mode; + int flags; + struct file *nfile; + struct snd_pcm_substream *rsubstream; + + if (get_user(dup_mode, (int __user *)arg)) + return -EFAULT; + if (dup_mode != 0) + return -ENOSYS; + flags = file->f_flags & (O_RDWR|O_NONBLOCK); + flags |= O_APPEND | O_CLOEXEC; + fd = get_unused_fd_flags(flags); + if (fd < 0) + return fd; + nfile = anon_inode_getfile("snd-pcm", file->f_op, NULL, flags); + if (IS_ERR(nfile)) { + put_unused_fd(fd); + return PTR_ERR(nfile); + } + fd_install(fd, nfile); + res = snd_pcm_open_substream(substream->pcm, substream->number, + nfile, &rsubstream); + if (res < 0) { + ksys_close(fd); + return res; + } + put_user(fd, (int __user *)arg); + return 0; +} + static int snd_pcm_common_ioctl(struct file *file, struct snd_pcm_substream *substream, unsigned int cmd, void __user *arg) @@ -2864,6 +2902,8 @@ static int snd_pcm_common_ioctl(struct file *file, (unsigned int __user *)arg)) return -EFAULT; return 0; + case SNDRV_PCM_IOCTL_ANONYMOUS_DUP: + return snd_pcm_anonymous_dup(file, substream, (int __user *)arg); case SNDRV_PCM_IOCTL_HW_REFINE: return snd_pcm_hw_refine_user(substream, arg); case SNDRV_PCM_IOCTL_HW_PARAMS:
On Mon, 28 Jan 2019 14:31:23 +0100, Jaroslav Kysela wrote:
Dne 25.1.2019 v 19:25 Mark Brown napsal(a):
On Fri, Jan 25, 2019 at 02:19:22PM +0100, Takashi Iwai wrote:
Leo Yan wrote:
If we directly use the device node /dev/snd/ as file descriptor, even though we specify flag O_EXCL when open it, but it still is not an anon inode file descriptor. Thus this is not safe enough and will be blocked by SELinux. On the other hand, this patch wants to use dma-buf framework to provide file descriptor for the audio buffer, and this audio buffer can be one of mutiple audio buffers in the system and it can be shared to any audio client program.
Hrm, it sounds like a workaround just to bypass SELinux check...
The sound server can open another PCM stream with O_APPEND, and pass that fd to the client, too?
So long as we can teach SELinux that they're safe to export, yeah.
It seems that SELinux works with the file, so the SELinux will block the fd pass, because the file descriptor (through standard dup()) continues to use the /dev/snd inode.
I would propose to implement a dup ioctl to return a new anon_inode:snd-pcm file descriptor (see bellow).
I like the idea. This would work around the messy issues gracefully, and more importantly, it's easier to maintain for us.
And the restriction of ioctls for anon dup should be fairly easy to implement on top of this.
Thanks!
Takashi
If we agree on this, I can propose the full solution.
-- Subject: [PATCH] ALSA: pcm: implement the anonymous dup (inode file descriptor)
This patch implements new SNDRV_PCM_IOCTL_ANONYMOUS_DUP ioctl which returns the new duplicated anonymous inode file descriptor (anon_inode:snd-pcm) which can be passed to the restricted clients.
This implementation is just a concept for comments - it does not contain the additional restriction control.
TODO: The clients might be restricted to disallow a set of controls (ioctls) for the audio stream.
This patch is meant to be the alternative for the dma-buf interface. Both implementation have some pros and cons:
anon_inode:dmabuf
- a bit standard export API for the DMA buffers
- fencing for the concurrent access [1]
- driver/kernel interface for the DMA buffer [1]
- multiple attach/detach scheme [1]
[1] the real usage for the sound PCM is unknown at the moment for this feature
anon_inode:snd-pcm
- simple (no problem with ref-counting, non-standard mmap implementation etc.)
- allow to use more sound interfaces for the file descriptor like status ioctls
- more fine grained security policies (another anon_inode name unshared with other drivers)
include/uapi/sound/asound.h | 1 + sound/core/pcm_compat.c | 1 + sound/core/pcm_native.c | 40 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+)
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 404d4b9ffe76..ad821a52f970 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -576,6 +576,7 @@ enum { #define SNDRV_PCM_IOCTL_TSTAMP _IOW('A', 0x02, int) #define SNDRV_PCM_IOCTL_TTSTAMP _IOW('A', 0x03, int) #define SNDRV_PCM_IOCTL_USER_PVERSION _IOW('A', 0x04, int) +#define SNDRV_PCM_IOCTL_ANONYMOUS_DUP _IOW('A', 0x05, int) #define SNDRV_PCM_IOCTL_HW_REFINE _IOWR('A', 0x10, struct snd_pcm_hw_params) #define SNDRV_PCM_IOCTL_HW_PARAMS _IOWR('A', 0x11, struct snd_pcm_hw_params) #define SNDRV_PCM_IOCTL_HW_FREE _IO('A', 0x12) diff --git a/sound/core/pcm_compat.c b/sound/core/pcm_compat.c index 946ab080ac00..22446cd574ee 100644 --- a/sound/core/pcm_compat.c +++ b/sound/core/pcm_compat.c @@ -675,6 +675,7 @@ static long snd_pcm_ioctl_compat(struct file *file, unsigned int cmd, unsigned l case SNDRV_PCM_IOCTL_TSTAMP: case SNDRV_PCM_IOCTL_TTSTAMP: case SNDRV_PCM_IOCTL_USER_PVERSION:
- case SNDRV_PCM_IOCTL_ANONYMOUS_DUP: case SNDRV_PCM_IOCTL_HWSYNC: case SNDRV_PCM_IOCTL_PREPARE: case SNDRV_PCM_IOCTL_RESET:
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 26afb6b0889a..a21bb482b4b0 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -37,6 +37,8 @@ #include <sound/minors.h> #include <linux/uio.h> #include <linux/delay.h> +#include <linux/anon_inodes.h> +#include <linux/syscalls.h>
#include "pcm_local.h"
@@ -2836,6 +2838,42 @@ static int snd_pcm_forward_ioctl(struct snd_pcm_substream *substream, return result < 0 ? result : 0; }
+static int snd_pcm_anonymous_dup(struct file *file,
struct snd_pcm_substream *substream,int __user *arg)+{
- int fd;
- int res;
- int dup_mode;
- int flags;
- struct file *nfile;
- struct snd_pcm_substream *rsubstream;
- if (get_user(dup_mode, (int __user *)arg))
return -EFAULT;- if (dup_mode != 0)
return -ENOSYS;- flags = file->f_flags & (O_RDWR|O_NONBLOCK);
- flags |= O_APPEND | O_CLOEXEC;
- fd = get_unused_fd_flags(flags);
- if (fd < 0)
return fd;- nfile = anon_inode_getfile("snd-pcm", file->f_op, NULL, flags);
- if (IS_ERR(nfile)) {
put_unused_fd(fd);return PTR_ERR(nfile);- }
- fd_install(fd, nfile);
- res = snd_pcm_open_substream(substream->pcm, substream->number,
nfile, &rsubstream);- if (res < 0) {
ksys_close(fd);return res;- }
- put_user(fd, (int __user *)arg);
- return 0;
+}
static int snd_pcm_common_ioctl(struct file *file, struct snd_pcm_substream *substream, unsigned int cmd, void __user *arg) @@ -2864,6 +2902,8 @@ static int snd_pcm_common_ioctl(struct file *file, (unsigned int __user *)arg)) return -EFAULT; return 0;
- case SNDRV_PCM_IOCTL_ANONYMOUS_DUP:
return snd_pcm_anonymous_dup(file, substream, (int __user *)arg);--
Jaroslav-- Jaroslav Kysela perex@perex.cz Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
participants (5)
-
Baolin Wang -
Jaroslav Kysela -
Leo Yan -
Mark Brown -
Takashi Iwai