[PATCH 0/2] ALSA: pcm: implement the anonymous dup v2

[PATCH 0/2] ALSA: pcm: implement the anonymous dup v2

[Jaroslav Kysela]

This patchset contains the anonymous dup implementation with permissions
checking for the ALSA's PCM interface in kernel to enable the restricted
DMA sound buffer sharing for the restricted tasks.

The code was tested through qemu and it seems to be pretty stable.

The initial tinyalsa implementation can be found here:

  https://github.com/perexg/tinyalsa/commits/anondup

The filtering might be refined. It depends on the real requirements.
Perhaps, we may create more ioctl groups. Any comments are more than
welcome.

v2 of the patches:

- change clone parameter to subdevice number for the pcm attach
- change SNDRV_PCM_PERM_MAX to SNDRV_PCM_PERM_MASK
- the tinyalsa implementation was a little updated (restructured)

Cc: Phil Burk <philburk@google.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Baolin Wang <baolin.wang@linaro.org>

Jaroslav Kysela (2):
  ALSA: pcm: implement the anonymous dup (inode file descriptor)
  ALSA: pcm: implement the ioctl/mmap filter for the anonymous dup

 include/sound/pcm.h         |   9 +--
 include/uapi/sound/asound.h |  12 +++-
 sound/core/oss/pcm_oss.c    |   2 +-
 sound/core/pcm.c            |  13 ++--
 sound/core/pcm_compat.c     |   1 +
 sound/core/pcm_native.c     | 153 ++++++++++++++++++++++++++++++++++++++++++--
 6 files changed, 171 insertions(+), 19 deletions(-)

-- 
2.13.6

[PATCH 1/2] ALSA: pcm: implement the anonymous dup (inode file descriptor)

[Jaroslav Kysela]

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 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)

Signed-off-by: Jaroslav Kysela <perex@perex.cz>
---
 include/sound/pcm.h         |  8 +++---
 include/uapi/sound/asound.h |  3 +-
 sound/core/oss/pcm_oss.c    |  2 +-
 sound/core/pcm.c            | 13 ++++-----
 sound/core/pcm_compat.c     |  1 +
 sound/core/pcm_native.c     | 70 +++++++++++++++++++++++++++++++++++++++++----
 6 files changed, 78 insertions(+), 19 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index ca20f80f8976..b79ffaa0241d 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -579,11 +579,11 @@ static inline int snd_pcm_suspend_all(struct snd_pcm *pcm)
 }
 #endif
 int snd_pcm_kernel_ioctl(struct snd_pcm_substream *substream, unsigned int cmd, void *arg);
-int snd_pcm_open_substream(struct snd_pcm *pcm, int stream, struct file *file,
-			   struct snd_pcm_substream **rsubstream);
+int snd_pcm_open_substream(struct snd_pcm *pcm, int stream, int subdevice,
+                           struct file *file, struct snd_pcm_substream **rsubstream);
 void snd_pcm_release_substream(struct snd_pcm_substream *substream);
-int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream, struct file *file,
-			     struct snd_pcm_substream **rsubstream);
+int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream, int subdevice,
+			     struct file *file, struct snd_pcm_substream **rsubstream);
 void snd_pcm_detach_substream(struct snd_pcm_substream *substream);
 int snd_pcm_mmap_data(struct snd_pcm_substream *substream, struct file *file, struct vm_area_struct *area);
 
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 404d4b9ffe76..ebc17d5a3490 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -153,7 +153,7 @@ struct snd_hwdep_dsp_image {
  *                                                                           *
  *****************************************************************************/
 
-#define SNDRV_PCM_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 14)
+#define SNDRV_PCM_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 15)
 
 typedef unsigned long snd_pcm_uframes_t;
 typedef signed long snd_pcm_sframes_t;
@@ -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   _IOWR('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/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c
index d5b0d7ba83c4..2ed609b65c45 100644
--- a/sound/core/oss/pcm_oss.c
+++ b/sound/core/oss/pcm_oss.c
@@ -2420,7 +2420,7 @@ static int snd_pcm_oss_open_file(struct file *file,
 			if (! (f_mode & FMODE_READ))
 				continue;
 		}
-		err = snd_pcm_open_substream(pcm, idx, file, &substream);
+		err = snd_pcm_open_substream(pcm, idx, -1, file, &substream);
 		if (err < 0) {
 			snd_pcm_oss_release_file(pcm_oss_file);
 			return err;
diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index 4f45b3000347..af6f7fc3687b 100644
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -918,15 +918,14 @@ static int snd_pcm_dev_free(struct snd_device *device)
 	return snd_pcm_free(pcm);
 }
 
-int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream,
-			     struct file *file,
+int snd_pcm_attach_substream(struct snd_pcm *pcm,
+			     int stream, int subdevice, struct file *file,
 			     struct snd_pcm_substream **rsubstream)
 {
 	struct snd_pcm_str * pstr;
 	struct snd_pcm_substream *substream;
 	struct snd_pcm_runtime *runtime;
 	struct snd_card *card;
-	int prefer_subdevice;
 	size_t size;
 
 	if (snd_BUG_ON(!pcm || !rsubstream))
@@ -940,7 +939,6 @@ int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream,
 		return -ENODEV;
 
 	card = pcm->card;
-	prefer_subdevice = snd_ctl_get_preferred_subdevice(card, SND_CTL_SUBDEV_PCM);
 
 	if (pcm->info_flags & SNDRV_PCM_INFO_HALF_DUPLEX) {
 		int opposite = !stream;
@@ -953,14 +951,14 @@ int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream,
 	}
 
 	if (file->f_flags & O_APPEND) {
-		if (prefer_subdevice < 0) {
+		if (subdevice < 0) {
 			if (pstr->substream_count > 1)
 				return -EINVAL; /* must be unique */
 			substream = pstr->substream;
 		} else {
 			for (substream = pstr->substream; substream;
 			     substream = substream->next)
-				if (substream->number == prefer_subdevice)
+				if (substream->number == subdevice)
 					break;
 		}
 		if (! substream)
@@ -974,8 +972,7 @@ int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream,
 
 	for (substream = pstr->substream; substream; substream = substream->next) {
 		if (!SUBSTREAM_BUSY(substream) &&
-		    (prefer_subdevice == -1 ||
-		     substream->number == prefer_subdevice))
+		    (subdevice == -1 || substream->number == subdevice))
 			break;
 	}
 	if (substream == NULL)
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 0bc4aa0ac9cf..8874a685f1e8 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"
 
@@ -2437,14 +2439,17 @@ void snd_pcm_release_substream(struct snd_pcm_substream *substream)
 }
 EXPORT_SYMBOL(snd_pcm_release_substream);
 
-int snd_pcm_open_substream(struct snd_pcm *pcm, int stream,
+int snd_pcm_open_substream(struct snd_pcm *pcm, int stream, int subdevice,
 			   struct file *file,
 			   struct snd_pcm_substream **rsubstream)
 {
 	struct snd_pcm_substream *substream;
 	int err;
 
-	err = snd_pcm_attach_substream(pcm, stream, file, &substream);
+	if (subdevice < 0 && pcm)
+		subdevice = snd_ctl_get_preferred_subdevice(pcm->card, SND_CTL_SUBDEV_PCM);
+
+	err = snd_pcm_attach_substream(pcm, stream, subdevice, file, &substream);
 	if (err < 0)
 		return err;
 	if (substream->ref_count > 1) {
@@ -2480,13 +2485,14 @@ EXPORT_SYMBOL(snd_pcm_open_substream);
 
 static int snd_pcm_open_file(struct file *file,
 			     struct snd_pcm *pcm,
-			     int stream)
+			     int stream,
+			     int subdevice)
 {
 	struct snd_pcm_file *pcm_file;
 	struct snd_pcm_substream *substream;
 	int err;
 
-	err = snd_pcm_open_substream(pcm, stream, file, &substream);
+	err = snd_pcm_open_substream(pcm, stream, subdevice, file, &substream);
 	if (err < 0)
 		return err;
 
@@ -2551,7 +2557,7 @@ static int snd_pcm_open(struct file *file, struct snd_pcm *pcm, int stream)
 	add_wait_queue(&pcm->open_wait, &wait);
 	mutex_lock(&pcm->open_mutex);
 	while (1) {
-		err = snd_pcm_open_file(file, pcm, stream);
+		err = snd_pcm_open_file(file, pcm, stream, -1);
 		if (err >= 0)
 			break;
 		if (err == -EAGAIN) {
@@ -2595,6 +2601,9 @@ static int snd_pcm_release(struct inode *inode, struct file *file)
 	struct snd_pcm_file *pcm_file;
 
 	pcm_file = file->private_data;
+	/* a problem in the anonymous dup can hit the NULL pcm_file */
+	if (pcm_file == NULL)
+		return 0;
 	substream = pcm_file->substream;
 	if (snd_BUG_ON(!substream))
 		return -ENXIO;
@@ -2878,6 +2887,55 @@ 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 err;
+	int perm;
+	int flags;
+	struct file *nfile;
+	struct snd_pcm *pcm = substream->pcm;
+
+	if (get_user(perm, (int __user *)arg))
+		return -EFAULT;
+	if (perm < 0)
+		return -EPERM;
+	flags = file->f_flags & (O_ACCMODE | 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);
+	}
+	/* anon_inode_getfile() filters the O_APPEND flag out */
+	nfile->f_flags |= O_APPEND;
+	fd_install(fd, nfile);
+	if (!try_module_get(pcm->card->module)) {
+		err = -EFAULT;
+		goto __error1;
+	}
+	err = snd_card_file_add(substream->pcm->card, nfile);
+	if (err < 0)
+		goto __error2;
+	err = snd_pcm_open_file(nfile, substream->pcm,
+				substream->stream, substream->number);
+	if (err >= 0) {
+		put_user(fd, (int __user *)arg);
+		return 0;
+	}
+	snd_card_file_remove(substream->pcm->card, nfile);
+      __error2:
+	module_put(pcm->card->module);
+      __error1:
+	ksys_close(fd);
+	return err;
+}
+
 static int snd_pcm_common_ioctl(struct file *file,
 				 struct snd_pcm_substream *substream,
 				 unsigned int cmd, void __user *arg)
@@ -2906,6 +2964,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:
-- 
2.13.6

[PATCH 2/2] ALSA: pcm: implement the ioctl/mmap filter for the anonymous dup

[Jaroslav Kysela]

Create seven control bits to allow the various restrictions for the
anonymous file descriptor.

Signed-off-by: Jaroslav Kysela <perex@perex.cz>
---
 include/sound/pcm.h         |  1 +
 include/uapi/sound/asound.h |  9 +++++
 sound/core/pcm_native.c     | 85 ++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 94 insertions(+), 1 deletion(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index b79ffaa0241d..e0469b2c1115 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -227,6 +227,7 @@ struct snd_pcm_ops {
 struct snd_pcm_file {
 	struct snd_pcm_substream *substream;
 	int no_compat_mmap;
+	unsigned int perm;		/* file descriptor permissions */
 	unsigned int user_pversion;	/* supported protocol version */
 };
 
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index ebc17d5a3490..8d99aa8916f0 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -571,6 +571,15 @@ enum {
 #define SNDRV_CHMAP_PHASE_INVERSE	(0x01 << 16)
 #define SNDRV_CHMAP_DRIVER_SPEC		(0x02 << 16)
 
+#define SNDRV_PCM_PERM_MMAP		(1<<0)
+#define SNDRV_PCM_PERM_MMAP_STATUS	(1<<1)
+#define SNDRV_PCM_PERM_MMAP_CONTROL	(1<<2)
+#define SNDRV_PCM_PERM_RW		(1<<3)
+#define SNDRV_PCM_PERM_CONTROL		(1<<4)
+#define SNDRV_PCM_PERM_STATUS		(1<<5)
+#define SNDRV_PCM_PERM_SYNC		(1<<6)
+#define SNDRV_PCM_PERM_MASK		((SNDRV_PCM_PERM_SYNC<<1)-1)
+
 #define SNDRV_PCM_IOCTL_PVERSION	_IOR('A', 0x00, int)
 #define SNDRV_PCM_IOCTL_INFO		_IOR('A', 0x01, struct snd_pcm_info)
 #define SNDRV_PCM_IOCTL_TSTAMP		_IOW('A', 0x02, int)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 8874a685f1e8..0fceed62b839 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2502,6 +2502,7 @@ static int snd_pcm_open_file(struct file *file,
 		return -ENOMEM;
 	}
 	pcm_file->substream = substream;
+	pcm_file->perm = ~0;
 	if (substream->ref_count == 1)
 		substream->pcm_release = pcm_release_private;
 	file->private_data = pcm_file;
@@ -2897,10 +2898,11 @@ static int snd_pcm_anonymous_dup(struct file *file,
 	int flags;
 	struct file *nfile;
 	struct snd_pcm *pcm = substream->pcm;
+	struct snd_pcm_file *pcm_file;
 
 	if (get_user(perm, (int __user *)arg))
 		return -EFAULT;
-	if (perm < 0)
+	if (perm & ~SNDRV_PCM_PERM_MASK)
 		return -EPERM;
 	flags = file->f_flags & (O_ACCMODE | O_NONBLOCK);
 	flags |= O_APPEND | O_CLOEXEC;
@@ -2925,6 +2927,8 @@ static int snd_pcm_anonymous_dup(struct file *file,
 	err = snd_pcm_open_file(nfile, substream->pcm,
 				substream->stream, substream->number);
 	if (err >= 0) {
+		pcm_file = nfile->private_data;
+		pcm_file->perm = perm;
 		put_user(fd, (int __user *)arg);
 		return 0;
 	}
@@ -2936,6 +2940,73 @@ static int snd_pcm_anonymous_dup(struct file *file,
 	return err;
 }
 
+static int snd_pcm_ioctl_check_perm(struct snd_pcm_file *pcm_file,
+				    unsigned int cmd)
+{
+	if (pcm_file->perm == ~0)
+		return 1;
+	/*
+	 * the setup, linking and anonymous dup is not allowed
+	 * for the restricted file descriptors
+	 */
+	switch (cmd) {
+	case SNDRV_PCM_IOCTL_PVERSION:
+	case SNDRV_PCM_IOCTL_INFO:
+	case SNDRV_PCM_IOCTL_USER_PVERSION:
+	case SNDRV_PCM_IOCTL_CHANNEL_INFO:
+		return 1;
+	}
+	if (pcm_file->perm & SNDRV_PCM_PERM_CONTROL) {
+		switch (cmd) {
+		case SNDRV_PCM_IOCTL_PREPARE:
+		case SNDRV_PCM_IOCTL_RESET:
+		case SNDRV_PCM_IOCTL_START:
+		case SNDRV_PCM_IOCTL_XRUN:
+		case SNDRV_PCM_IOCTL_RESUME:
+		case SNDRV_PCM_IOCTL_DRAIN:
+		case SNDRV_PCM_IOCTL_DROP:
+		case SNDRV_PCM_IOCTL_PAUSE:
+			return 1;
+		default:
+			break;
+		}
+	}
+	if (pcm_file->perm & SNDRV_PCM_PERM_STATUS) {
+		switch (cmd) {
+		case SNDRV_PCM_IOCTL_STATUS:
+		case SNDRV_PCM_IOCTL_STATUS_EXT:
+		case SNDRV_PCM_IOCTL_DELAY:
+			return 1;
+		default:
+			break;
+		}
+	}
+	if (pcm_file->perm & SNDRV_PCM_PERM_SYNC) {
+		switch (cmd) {
+		case SNDRV_PCM_IOCTL_HWSYNC:
+		case SNDRV_PCM_IOCTL_SYNC_PTR:
+		case SNDRV_PCM_IOCTL_REWIND:
+		case SNDRV_PCM_IOCTL_FORWARD:
+			return 1;
+		default:
+			break;
+		}
+	}
+	if (pcm_file->perm & SNDRV_PCM_PERM_RW) {
+		switch (cmd) {
+		case SNDRV_PCM_IOCTL_WRITEI_FRAMES:
+		case SNDRV_PCM_IOCTL_READI_FRAMES:
+		case SNDRV_PCM_IOCTL_WRITEN_FRAMES:
+		case SNDRV_PCM_IOCTL_READN_FRAMES:
+			return 1;
+		default:
+			break;
+		}
+	}
+
+	return 0;
+}
+
 static int snd_pcm_common_ioctl(struct file *file,
 				 struct snd_pcm_substream *substream,
 				 unsigned int cmd, void __user *arg)
@@ -2950,6 +3021,9 @@ static int snd_pcm_common_ioctl(struct file *file,
 	if (res < 0)
 		return res;
 
+	if (!snd_pcm_ioctl_check_perm(pcm_file, cmd))
+		return -EPERM;
+
 	switch (cmd) {
 	case SNDRV_PCM_IOCTL_PVERSION:
 		return put_user(SNDRV_PCM_VERSION, (int __user *)arg) ? -EFAULT : 0;
@@ -3298,6 +3372,9 @@ static int snd_pcm_mmap_status(struct snd_pcm_substream *substream, struct file
 			       struct vm_area_struct *area)
 {
 	long size;
+	struct snd_pcm_file *pcm_file = file->private_data;
+	if (!(pcm_file->perm & SNDRV_PCM_PERM_MMAP_STATUS))
+		return -EPERM;
 	if (!(area->vm_flags & VM_READ))
 		return -EINVAL;
 	size = area->vm_end - area->vm_start;
@@ -3334,6 +3411,9 @@ static int snd_pcm_mmap_control(struct snd_pcm_substream *substream, struct file
 				struct vm_area_struct *area)
 {
 	long size;
+	struct snd_pcm_file *pcm_file = file->private_data;
+	if (!(pcm_file->perm & SNDRV_PCM_PERM_MMAP_CONTROL))
+		return -EPERM;
 	if (!(area->vm_flags & VM_READ))
 		return -EINVAL;
 	size = area->vm_end - area->vm_start;
@@ -3508,11 +3588,14 @@ int snd_pcm_mmap_data(struct snd_pcm_substream *substream, struct file *file,
 		      struct vm_area_struct *area)
 {
 	struct snd_pcm_runtime *runtime;
+	struct snd_pcm_file *pcm_file = file->private_data;
 	long size;
 	unsigned long offset;
 	size_t dma_bytes;
 	int err;
 
+	if (!(pcm_file->perm & SNDRV_PCM_PERM_MMAP))
+		return -EPERM;
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 		if (!(area->vm_flags & (VM_WRITE|VM_READ)))
 			return -EINVAL;
-- 
2.13.6

Re: [PATCH 2/2] ALSA: pcm: implement the ioctl/mmap filter for the anonymous dup

[Takashi Iwai]

On Wed, 30 Jan 2019 09:47:33 +0100,
Jaroslav Kysela wrote:
> 
> Create seven control bits to allow the various restrictions for the
> anonymous file descriptor.
> 
> Signed-off-by: Jaroslav Kysela <perex@perex.cz>

I think we need to check SNDRV_PCM_PERM_RW for read/write ops, too.

Other than that, looks good to me.

  Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

> ---
>  include/sound/pcm.h         |  1 +
>  include/uapi/sound/asound.h |  9 +++++
>  sound/core/pcm_native.c     | 85 ++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 94 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index b79ffaa0241d..e0469b2c1115 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -227,6 +227,7 @@ struct snd_pcm_ops {
>  struct snd_pcm_file {
>  	struct snd_pcm_substream *substream;
>  	int no_compat_mmap;
> +	unsigned int perm;		/* file descriptor permissions */
>  	unsigned int user_pversion;	/* supported protocol version */
>  };
>  
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index ebc17d5a3490..8d99aa8916f0 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -571,6 +571,15 @@ enum {
>  #define SNDRV_CHMAP_PHASE_INVERSE	(0x01 << 16)
>  #define SNDRV_CHMAP_DRIVER_SPEC		(0x02 << 16)
>  
> +#define SNDRV_PCM_PERM_MMAP		(1<<0)
> +#define SNDRV_PCM_PERM_MMAP_STATUS	(1<<1)
> +#define SNDRV_PCM_PERM_MMAP_CONTROL	(1<<2)
> +#define SNDRV_PCM_PERM_RW		(1<<3)
> +#define SNDRV_PCM_PERM_CONTROL		(1<<4)
> +#define SNDRV_PCM_PERM_STATUS		(1<<5)
> +#define SNDRV_PCM_PERM_SYNC		(1<<6)
> +#define SNDRV_PCM_PERM_MASK		((SNDRV_PCM_PERM_SYNC<<1)-1)
> +
>  #define SNDRV_PCM_IOCTL_PVERSION	_IOR('A', 0x00, int)
>  #define SNDRV_PCM_IOCTL_INFO		_IOR('A', 0x01, struct snd_pcm_info)
>  #define SNDRV_PCM_IOCTL_TSTAMP		_IOW('A', 0x02, int)
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 8874a685f1e8..0fceed62b839 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -2502,6 +2502,7 @@ static int snd_pcm_open_file(struct file *file,
>  		return -ENOMEM;
>  	}
>  	pcm_file->substream = substream;
> +	pcm_file->perm = ~0;
>  	if (substream->ref_count == 1)
>  		substream->pcm_release = pcm_release_private;
>  	file->private_data = pcm_file;
> @@ -2897,10 +2898,11 @@ static int snd_pcm_anonymous_dup(struct file *file,
>  	int flags;
>  	struct file *nfile;
>  	struct snd_pcm *pcm = substream->pcm;
> +	struct snd_pcm_file *pcm_file;
>  
>  	if (get_user(perm, (int __user *)arg))
>  		return -EFAULT;
> -	if (perm < 0)
> +	if (perm & ~SNDRV_PCM_PERM_MASK)
>  		return -EPERM;
>  	flags = file->f_flags & (O_ACCMODE | O_NONBLOCK);
>  	flags |= O_APPEND | O_CLOEXEC;
> @@ -2925,6 +2927,8 @@ static int snd_pcm_anonymous_dup(struct file *file,
>  	err = snd_pcm_open_file(nfile, substream->pcm,
>  				substream->stream, substream->number);
>  	if (err >= 0) {
> +		pcm_file = nfile->private_data;
> +		pcm_file->perm = perm;
>  		put_user(fd, (int __user *)arg);
>  		return 0;
>  	}
> @@ -2936,6 +2940,73 @@ static int snd_pcm_anonymous_dup(struct file *file,
>  	return err;
>  }
>  
> +static int snd_pcm_ioctl_check_perm(struct snd_pcm_file *pcm_file,
> +				    unsigned int cmd)
> +{
> +	if (pcm_file->perm == ~0)
> +		return 1;
> +	/*
> +	 * the setup, linking and anonymous dup is not allowed
> +	 * for the restricted file descriptors
> +	 */
> +	switch (cmd) {
> +	case SNDRV_PCM_IOCTL_PVERSION:
> +	case SNDRV_PCM_IOCTL_INFO:
> +	case SNDRV_PCM_IOCTL_USER_PVERSION:
> +	case SNDRV_PCM_IOCTL_CHANNEL_INFO:
> +		return 1;
> +	}
> +	if (pcm_file->perm & SNDRV_PCM_PERM_CONTROL) {
> +		switch (cmd) {
> +		case SNDRV_PCM_IOCTL_PREPARE:
> +		case SNDRV_PCM_IOCTL_RESET:
> +		case SNDRV_PCM_IOCTL_START:
> +		case SNDRV_PCM_IOCTL_XRUN:
> +		case SNDRV_PCM_IOCTL_RESUME:
> +		case SNDRV_PCM_IOCTL_DRAIN:
> +		case SNDRV_PCM_IOCTL_DROP:
> +		case SNDRV_PCM_IOCTL_PAUSE:
> +			return 1;
> +		default:
> +			break;
> +		}
> +	}
> +	if (pcm_file->perm & SNDRV_PCM_PERM_STATUS) {
> +		switch (cmd) {
> +		case SNDRV_PCM_IOCTL_STATUS:
> +		case SNDRV_PCM_IOCTL_STATUS_EXT:
> +		case SNDRV_PCM_IOCTL_DELAY:
> +			return 1;
> +		default:
> +			break;
> +		}
> +	}
> +	if (pcm_file->perm & SNDRV_PCM_PERM_SYNC) {
> +		switch (cmd) {
> +		case SNDRV_PCM_IOCTL_HWSYNC:
> +		case SNDRV_PCM_IOCTL_SYNC_PTR:
> +		case SNDRV_PCM_IOCTL_REWIND:
> +		case SNDRV_PCM_IOCTL_FORWARD:
> +			return 1;
> +		default:
> +			break;
> +		}
> +	}
> +	if (pcm_file->perm & SNDRV_PCM_PERM_RW) {
> +		switch (cmd) {
> +		case SNDRV_PCM_IOCTL_WRITEI_FRAMES:
> +		case SNDRV_PCM_IOCTL_READI_FRAMES:
> +		case SNDRV_PCM_IOCTL_WRITEN_FRAMES:
> +		case SNDRV_PCM_IOCTL_READN_FRAMES:
> +			return 1;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int snd_pcm_common_ioctl(struct file *file,
>  				 struct snd_pcm_substream *substream,
>  				 unsigned int cmd, void __user *arg)
> @@ -2950,6 +3021,9 @@ static int snd_pcm_common_ioctl(struct file *file,
>  	if (res < 0)
>  		return res;
>  
> +	if (!snd_pcm_ioctl_check_perm(pcm_file, cmd))
> +		return -EPERM;
> +
>  	switch (cmd) {
>  	case SNDRV_PCM_IOCTL_PVERSION:
>  		return put_user(SNDRV_PCM_VERSION, (int __user *)arg) ? -EFAULT : 0;
> @@ -3298,6 +3372,9 @@ static int snd_pcm_mmap_status(struct snd_pcm_substream *substream, struct file
>  			       struct vm_area_struct *area)
>  {
>  	long size;
> +	struct snd_pcm_file *pcm_file = file->private_data;
> +	if (!(pcm_file->perm & SNDRV_PCM_PERM_MMAP_STATUS))
> +		return -EPERM;
>  	if (!(area->vm_flags & VM_READ))
>  		return -EINVAL;
>  	size = area->vm_end - area->vm_start;
> @@ -3334,6 +3411,9 @@ static int snd_pcm_mmap_control(struct snd_pcm_substream *substream, struct file
>  				struct vm_area_struct *area)
>  {
>  	long size;
> +	struct snd_pcm_file *pcm_file = file->private_data;
> +	if (!(pcm_file->perm & SNDRV_PCM_PERM_MMAP_CONTROL))
> +		return -EPERM;
>  	if (!(area->vm_flags & VM_READ))
>  		return -EINVAL;
>  	size = area->vm_end - area->vm_start;
> @@ -3508,11 +3588,14 @@ int snd_pcm_mmap_data(struct snd_pcm_substream *substream, struct file *file,
>  		      struct vm_area_struct *area)
>  {
>  	struct snd_pcm_runtime *runtime;
> +	struct snd_pcm_file *pcm_file = file->private_data;
>  	long size;
>  	unsigned long offset;
>  	size_t dma_bytes;
>  	int err;
>  
> +	if (!(pcm_file->perm & SNDRV_PCM_PERM_MMAP))
> +		return -EPERM;
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>  		if (!(area->vm_flags & (VM_WRITE|VM_READ)))
>  			return -EINVAL;
> -- 
> 2.13.6
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 
o

Re: [PATCH 1/2] ALSA: pcm: implement the anonymous dup (inode file descriptor)

[Takashi Iwai]

On Wed, 30 Jan 2019 09:47:32 +0100,
Jaroslav Kysela wrote:
> 
> 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 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)
> 
> Signed-off-by: Jaroslav Kysela <perex@perex.cz>

This version is indeed shorter, that's good!

Just minor nitpicking:
> +static int snd_pcm_anonymous_dup(struct file *file,
> +				 struct snd_pcm_substream *substream,
> +				 int __user *arg)
> +{
> +	int fd;
> +	int err;
> +	int perm;
> +	int flags;
> +	struct file *nfile;
> +	struct snd_pcm *pcm = substream->pcm;

You prefer rather a normal Christmas tree? ;)

> +	if (get_user(perm, (int __user *)arg))

The cast looks superfluous.

> +		return -EFAULT;
> +	if (perm < 0)
> +		return -EPERM;
> +	flags = file->f_flags & (O_ACCMODE | 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);
> +	}
> +	/* anon_inode_getfile() filters the O_APPEND flag out */
> +	nfile->f_flags |= O_APPEND;
> +	fd_install(fd, nfile);
> +	if (!try_module_get(pcm->card->module)) {
> +		err = -EFAULT;

EFAULT doesn't appear to be the best fitting for this error path.

> +		goto __error1;
> +	}
> +	err = snd_card_file_add(substream->pcm->card, nfile);

substream->pcm can be replaced with pcm.

> +	if (err < 0)
> +		goto __error2;
> +	err = snd_pcm_open_file(nfile, substream->pcm,

Ditto.

> +				substream->stream, substream->number);
> +	if (err >= 0) {
> +		put_user(fd, (int __user *)arg);

The cast looks superfluous.

> +		return 0;
> +	}
> +	snd_card_file_remove(substream->pcm->card, nfile);

Ditto.


Other than these small things, LGTM;
  Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

Re: [PATCH 2/2] ALSA: pcm: implement the ioctl/mmap filter for the anonymous dup

[Jaroslav Kysela]

Dne 30.1.2019 v 12:27 Takashi Iwai napsal(a):
> On Wed, 30 Jan 2019 09:47:33 +0100,
> Jaroslav Kysela wrote:
>>
>> Create seven control bits to allow the various restrictions for the
>> anonymous file descriptor.
>>
>> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
> 
> I think we need to check SNDRV_PCM_PERM_RW for read/write ops, too.

Yes, it will be in v3. Thanks for the review.

				Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH 1/2] ALSA: pcm: implement the anonymous dup (inode file descriptor)

[Jaroslav Kysela]

Dne 30.1.2019 v 12:37 Takashi Iwai napsal(a):
> On Wed, 30 Jan 2019 09:47:32 +0100,
> Jaroslav Kysela wrote:
>>
>> 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.

> +	if (!try_module_get(pcm->card->module)) {
>> +		err = -EFAULT;
> 
> EFAULT doesn't appear to be the best fitting for this error path.

It's in the sync with snd_pcm_open(). This error path should not be
activated in the regular environment anyway (so it's real FAULT somewhere).
If it have to be changed, let change all locations in a separate patch.

Thanks for the review.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.