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

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

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

v3 of the patches:

- group integer declarations in snd_pcm_anonymous_dup()
- replaced substream->pcm with pcm in snd_pcm_anonymous_dup()
- added SNDRV_PCM_PERM_RW check for read/write/readv/writev syscalls

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     | 158 ++++++++++++++++++++++++++++++++++++++++++--
 6 files changed, 176 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>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
---
 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     | 67 +++++++++++++++++++++++++++++++++++++++++----
 6 files changed, 75 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..bb14658e4482 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,52 @@ 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, err, perm, flags;
+	struct file *nfile;
+	struct snd_pcm *pcm = substream->pcm;
+
+	if (get_user(perm, 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(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, arg);
+		return 0;
+	}
+	snd_card_file_remove(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 +2961,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>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/pcm.h         |  1 +
 include/uapi/sound/asound.h |  9 +++++
 sound/core/pcm_native.c     | 93 ++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 102 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 bb14658e4482..57512f7131f2 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;
@@ -2894,10 +2895,11 @@ static int snd_pcm_anonymous_dup(struct file *file,
 	int fd, err, perm, flags;
 	struct file *nfile;
 	struct snd_pcm *pcm = substream->pcm;
+	struct snd_pcm_file *pcm_file;
 
 	if (get_user(perm, 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;
@@ -2922,6 +2924,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, arg);
 		return 0;
 	}
@@ -2933,6 +2937,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)
@@ -2947,6 +3018,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;
@@ -3108,6 +3182,8 @@ static ssize_t snd_pcm_read(struct file *file, char __user *buf, size_t count,
 	substream = pcm_file->substream;
 	if (PCM_RUNTIME_CHECK(substream))
 		return -ENXIO;
+	if (!(pcm_file->perm & SNDRV_PCM_PERM_RW))
+		return -EPERM;
 	runtime = substream->runtime;
 	if (runtime->status->state == SNDRV_PCM_STATE_OPEN)
 		return -EBADFD;
@@ -3132,6 +3208,8 @@ static ssize_t snd_pcm_write(struct file *file, const char __user *buf,
 	substream = pcm_file->substream;
 	if (PCM_RUNTIME_CHECK(substream))
 		return -ENXIO;
+	if (!(pcm_file->perm & SNDRV_PCM_PERM_RW))
+		return -EPERM;
 	runtime = substream->runtime;
 	if (runtime->status->state == SNDRV_PCM_STATE_OPEN)
 		return -EBADFD;
@@ -3158,6 +3236,8 @@ static ssize_t snd_pcm_readv(struct kiocb *iocb, struct iov_iter *to)
 	substream = pcm_file->substream;
 	if (PCM_RUNTIME_CHECK(substream))
 		return -ENXIO;
+	if (!(pcm_file->perm & SNDRV_PCM_PERM_RW))
+		return -EPERM;
 	runtime = substream->runtime;
 	if (runtime->status->state == SNDRV_PCM_STATE_OPEN)
 		return -EBADFD;
@@ -3194,6 +3274,8 @@ static ssize_t snd_pcm_writev(struct kiocb *iocb, struct iov_iter *from)
 	substream = pcm_file->substream;
 	if (PCM_RUNTIME_CHECK(substream))
 		return -ENXIO;
+	if (!(pcm_file->perm & SNDRV_PCM_PERM_RW))
+		return -EPERM;
 	runtime = substream->runtime;
 	if (runtime->status->state == SNDRV_PCM_STATE_OPEN)
 		return -EBADFD;
@@ -3295,6 +3377,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;
@@ -3331,6 +3416,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;
@@ -3505,11 +3593,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 0/2] ALSA: pcm: implement the anonymous dup v3

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1196 bytes --]

On Wed, Jan 30, 2019 at 01:41:37PM +0100, Jaroslav Kysela wrote:
> 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.

My understanding based on some off-list discussion is that the Android
security people are going to see anything that involves passing more
than a block of memory (and in particular anything that gives access to
the sound APIs) as a problem.  That's obviously going to be an issue for
anything O_APPEND based.  My understanding is that this is fundamentally
a risk mitigation thing - by not having any of the sound kernel
interfaces available to the applications affected there's no possibility
that any problems in the sound code can cause security issues.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Phil Burk]

Hello Mark,

Our security team was very concerned about the old ALSA FD. It provided too
much access to the guts of ALSA.

I assume they will not like anything other than a plain
"anon_inode:dmabuf". If it is a new FD, then the code would have to be
reviewed. Even if it looked OK there might be some holes that we don't
find. So it would probably be rejected.

I cannot speak for our security team so I am working on setting up a
meeting or conversation between Mark and Zach, our security expert.

Adding the anon_inode:snd-pcm might be great for ALSA. That could be used
by the HAL for STATUS and CONTROL. But it is likely that we will need an
additional anon_inode:dmabuf FD that is only associated with the PCM
buffer. It can then be safely passed to an Android app.

Thanks,
Phil Burk


On Wed, Jan 30, 2019 at 2:32 PM Mark Brown <broonie@kernel.org> wrote:

> On Wed, Jan 30, 2019 at 01:41:37PM +0100, Jaroslav Kysela wrote:
> > 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.
>
> My understanding based on some off-list discussion is that the Android
> security people are going to see anything that involves passing more
> than a block of memory (and in particular anything that gives access to
> the sound APIs) as a problem.  That's obviously going to be an issue for
> anything O_APPEND based.  My understanding is that this is fundamentally
> a risk mitigation thing - by not having any of the sound kernel
> interfaces available to the applications affected there's no possibility
> that any problems in the sound code can cause security issues.
>

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

[Leo Yan]

Hi Phil & all,

On Wed, Jan 30, 2019 at 04:45:04PM -0800, Phil Burk wrote:
> Hello Mark,
> 
> Our security team was very concerned about the old ALSA FD. It provided too
> much access to the guts of ALSA.
> 
> I assume they will not like anything other than a plain
> "anon_inode:dmabuf". If it is a new FD, then the code would have to be
> reviewed. Even if it looked OK there might be some holes that we don't
> find. So it would probably be rejected.
> 
> I cannot speak for our security team so I am working on setting up a
> meeting or conversation between Mark and Zach, our security expert.
> 
> Adding the anon_inode:snd-pcm might be great for ALSA. That could be used
> by the HAL for STATUS and CONTROL. But it is likely that we will need an
> additional anon_inode:dmabuf FD that is only associated with the PCM
> buffer. It can then be safely passed to an Android app.

Thanks for the inputs.  I went through discussions, I'd like to divide the
work into two mainly parts:

- The first part is to use dmabuf to dynamically import buffer to
  sound device; so the buffer is not bound to sound device at the
  initialization phase;

- The second part is to use dmabuf to export buffer with anon_inode;
  thus it can meet the security requirement.

I go through Jaroslav implementation (thanks a lot for the quick moving
for this part!), it tries to implement the second part, but it misses
the first part support for dynamically binding audio buffer; and
as Mark/Phil mentioned, Jaroslav patch tries to use the same one FD
for both sound device and audio buffer.

I think it's good to firstly use one test case to demonstrate to
dynamically import buffer to sound device, then this buffer can be
exported with anon_inode for user space.  Is this doable?

Thanks,
Leo Yan

> On Wed, Jan 30, 2019 at 2:32 PM Mark Brown <broonie@kernel.org> wrote:
> 
> > On Wed, Jan 30, 2019 at 01:41:37PM +0100, Jaroslav Kysela wrote:
> > > 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.
> >
> > My understanding based on some off-list discussion is that the Android
> > security people are going to see anything that involves passing more
> > than a block of memory (and in particular anything that gives access to
> > the sound APIs) as a problem.  That's obviously going to be an issue for
> > anything O_APPEND based.  My understanding is that this is fundamentally
> > a risk mitigation thing - by not having any of the sound kernel
> > interfaces available to the applications affected there's no possibility
> > that any problems in the sound code can cause security issues.
> >

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

[Takashi Iwai]

On Wed, 30 Jan 2019 23:32:37 +0100,
Mark Brown wrote:
> 
> On Wed, Jan 30, 2019 at 01:41:37PM +0100, Jaroslav Kysela wrote:
> > 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.
> 
> My understanding based on some off-list discussion is that the Android
> security people are going to see anything that involves passing more
> than a block of memory (and in particular anything that gives access to
> the sound APIs) as a problem.  That's obviously going to be an issue for
> anything O_APPEND based.  My understanding is that this is fundamentally
> a risk mitigation thing - by not having any of the sound kernel
> interfaces available to the applications affected there's no possibility
> that any problems in the sound code can cause security issues.

The patch 2 implements exactly that kind of access restriction, so
that the passed fd won't do anything else than wished.

If we want to be super-conservative, the implementation could be even
simpler -- instead of filtering, we may pass a minimum fd ops that
contains only mmap and release for the anon-dup fd...


thanks,

Takashi

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

[Takashi Iwai]

On Thu, 31 Jan 2019 01:45:04 +0100,
Phil Burk wrote:
> 
> Hello Mark,
> 
> Our security team was very concerned about the old ALSA FD. It provided too
> much access to the guts of ALSA. 
> 
> I assume they will not like anything other than a plain "anon_inode:dmabuf".
> If it is a new FD, then the code would have to be reviewed. Even if it looked
> OK there might be some holes that we don't find. So it would probably be
> rejected.

The review is appreciated, sure! ;)

Above all, it'd be helpful if you can tell us exactly which feature is
requested and exactly what have to be avoided.  Jaroslav's patchset
tries to provide the generic implementation, and this can do more than
you guys needed.  But it can restrict the permissions well, too.

> I cannot speak for our security team so I am working on setting up a meeting
> or conversation between Mark and Zach, our security expert.
> 
> Adding the anon_inode:snd-pcm might be great for ALSA. That could be used by
> the HAL for STATUS and CONTROL. But it is likely that we will need an
> additional anon_inode:dmabuf FD that is only associated with the PCM buffer.
> It can then be safely passed to an Android app.

I find it fine to add a dma-buf implementation, too -- but only if
it's really safely, sanely and simply implemented.  The suggested
implementation so far has a way too many holes, unfortunately, and it
can easily lead to kernel Oops when something wrong happens in the
master stream side.  And we need to cover some corner cases (e.g. a
hardware buffer setup that isn't supposed to be shared) that are
overlooked, too.


thanks,

Takashi

> 
> Thanks,
> Phil Burk
> 
> On Wed, Jan 30, 2019 at 2:32 PM Mark Brown <broonie@kernel.org> wrote:
> 
>     On Wed, Jan 30, 2019 at 01:41:37PM +0100, Jaroslav Kysela wrote:
>     > 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.
>    
>     My understanding based on some off-list discussion is that the Android
>     security people are going to see anything that involves passing more
>     than a block of memory (and in particular anything that gives access to
>     the sound APIs) as a problem.  That's obviously going to be an issue for
>     anything O_APPEND based.  My understanding is that this is fundamentally
>     a risk mitigation thing - by not having any of the sound kernel
>     interfaces available to the applications affected there's no possibility
>     that any problems in the sound code can cause security issues.
> 
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Jaroslav Kysela]

Dne 31.1.2019 v 01:45 Phil Burk napsal(a):
> Hello Mark,
> 
> Our security team was very concerned about the old ALSA FD. It provided
> too much access to the guts of ALSA. 
> 
> I assume they will not like anything other than a plain
> "anon_inode:dmabuf". If it is a new FD, then the code would have to be
> reviewed. Even if it looked OK there might be some holes that we don't
> find. So it would probably be rejected.

Hello Phil,

My point is that the dma-buf -> sound pcm buffer maping interface is
more complex, error prone and the code review/audit expensive than
reusing the current code without any functionality or security benefits.

We can nicely restrict the file operations to allow to mmap only the pcm
sound buffer and eventually, if we are too much paranoid (to bypass the
the bitmap like permission checking as I suggested), we can create a
special case for the Android usage to return the file descriptor with
very restricted 'struct file_operations' with just the mmap and release
callbacks. We can also change the name for this file descriptor to
distinguish it from the "anon_inode:snd-pcm" (for example
"anon_inode:snd-pcm-paranoid") to let SELinux do it's work properly.

The mmap implementation for the sound driver is few lines of the code
(for the standard devices - very easy to review), so we cannot speak
about security holes at all. If there is a problem with the kernel page
allocation/management in the sound driver, there will be problem with
dmabuf -> sound pcm buffer mapping, too (plus other problems caused by
the concurrent access to the buffer which is managed /alloc/free/ by the
sound driver - not dma-buf).

> I cannot speak for our security team so I am working on setting up a
> meeting or conversation between Mark and Zach, our security expert.

Thanks. Let us know the result. Eventually, your security expert can
freely join to our conversation here.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 758 bytes --]

On Thu, Jan 31, 2019 at 09:08:04AM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > anything O_APPEND based.  My understanding is that this is fundamentally
> > a risk mitigation thing - by not having any of the sound kernel
> > interfaces available to the applications affected there's no possibility
> > that any problems in the sound code can cause security issues.

> The patch 2 implements exactly that kind of access restriction, so
> that the passed fd won't do anything else than wished.

Yeah.

> If we want to be super-conservative, the implementation could be even
> simpler -- instead of filtering, we may pass a minimum fd ops that
> contains only mmap and release for the anon-dup fd...

I think that'd definitely help address the concerns.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Jaroslav Kysela]

Dne 31.1.2019 v 13:26 Mark Brown napsal(a):
> On Thu, Jan 31, 2019 at 09:08:04AM +0100, Takashi Iwai wrote:
>> Mark Brown wrote:
> 
>>> anything O_APPEND based.  My understanding is that this is fundamentally
>>> a risk mitigation thing - by not having any of the sound kernel
>>> interfaces available to the applications affected there's no possibility
>>> that any problems in the sound code can cause security issues.
> 
>> The patch 2 implements exactly that kind of access restriction, so
>> that the passed fd won't do anything else than wished.
> 
> Yeah.
> 
>> If we want to be super-conservative, the implementation could be even
>> simpler -- instead of filtering, we may pass a minimum fd ops that
>> contains only mmap and release for the anon-dup fd...
> 
> I think that'd definitely help address the concerns.

A possible implementation:

http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=ca15bc69a984cc0eae2c43d0a49c66a20c937f39

				Jaroslav

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

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

[Phil Burk]

+Zach Riggle <riggle@google.com>

Hello,

> Eventually, your security expert can freely join to our conversation here.

Thanks. The biggest unanswered question is whether Android security will
allow the file descriptor to be passed to an app. So I have added our
security person, Zach Riggle, who originally requested the
anon_inode:dmabuf FD.  If Zach is happy then I am happy.

We will need two file descriptors, one with full permissions for the HAL,
and one with only PCM access for the app to use.
It seems we are considering two options for the app's FD:
1) provide an anon_inode:dmabuf that never has CONTROL permissions, which
seems safe, but requires more changes to the driver and is a bit of a hack
2) provide an anon_inode:snd-pcm  that has CONTROL permissions turned off,
which seems seems less safe, but requires fewer changes and fits with the
design

Which one is actually better for security?

Here is an earlier argument for snd-pcm from Jaroslav:

My point is that the dma-buf -> sound pcm buffer maping interface is
> more complex, error prone and the code review/audit expensive than
> reusing the current code without any functionality or security benefits.
>


We can nicely restrict the file operations to allow to mmap only the pcm
> sound buffer and eventually, if we are too much paranoid (to bypass the
> the bitmap like permission checking as I suggested), we can create a
> special case for the Android usage to return the file descriptor with
> very restricted 'struct file_operations' with just the mmap and release
> callbacks. We can also change the name for this file descriptor to
> distinguish it from the "anon_inode:snd-pcm" (for example
> "anon_inode:snd-pcm-paranoid") to let SELinux do it's work properly.
>


The mmap implementation for the sound driver is few lines of the code
> (for the standard devices - very easy to review), so we cannot speak
> about security holes at all. If there is a problem with the kernel page
> allocation/management in the sound driver, there will be problem with
> dmabuf -> sound pcm buffer mapping, too (plus other problems caused by
> the concurrent access to the buffer which is managed /alloc/free/ by the
> sound driver - not dma-buf).


Also note that my emails bounce off the alsa-project mail list.

Phil Burk

On Thu, Jan 31, 2019 at 5:30 AM Jaroslav Kysela <perex@perex.cz> wrote:

> Dne 31.1.2019 v 13:26 Mark Brown napsal(a):
> > On Thu, Jan 31, 2019 at 09:08:04AM +0100, Takashi Iwai wrote:
> >> Mark Brown wrote:
> >
> >>> anything O_APPEND based.  My understanding is that this is
> fundamentally
> >>> a risk mitigation thing - by not having any of the sound kernel
> >>> interfaces available to the applications affected there's no
> possibility
> >>> that any problems in the sound code can cause security issues.
> >
> >> The patch 2 implements exactly that kind of access restriction, so
> >> that the passed fd won't do anything else than wished.
> >
> > Yeah.
> >
> >> If we want to be super-conservative, the implementation could be even
> >> simpler -- instead of filtering, we may pass a minimum fd ops that
> >> contains only mmap and release for the anon-dup fd...
> >
> > I think that'd definitely help address the concerns.
>
> A possible implementation:
>
>
> http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=ca15bc69a984cc0eae2c43d0a49c66a20c937f39
>
>                                 Jaroslav
>
> --
> Jaroslav Kysela <perex@perex.cz>
> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
>

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

[Phil Burk]

Mark and Zach and I talked.

Zach said that "dmabuf" is not a hard requirement. Another "anon_inode"
would probably be OK as long as the app cannot turn on any permissions
besides PCM access. Our security team will just need to review the changes.

So I think you should proceed with the "anon_inode:snd-pcm" if you think
that will be more secure. Thanks for proposing this.

Zach has some notes in his initial review of Jaroslav's code. Zach?

One thing Zach mentioned is that the API should only allow *removing*
permissions and not adding permissions.

What permissions would be set on the FD given to the app?

Also Mark mentioned that the FD app would have PCM access and "close"
permission. What flag allows close? What else is permitted under that flag?
Or is close permission  just a generic "FD" permission unrelated to ALSA?

Thanks for all your work on this. Sorry if I caused alarm. I just wanted to
make sure we could use the solution you provide.

Phil Burk

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

[Zach Riggle 🖖]

The big concerns on our end are:

(1) Can the buffer be mremap()ed with a different *offset* into the
buffer?  This was a concern in the past and the reason for the anon_inode
stuff at all.  I believe that as long as the *size* of the mapping doesn't
change, Linux mm will gladly permit mremap() without informing the driver.
(2) Can we ensure that permissions can only ever be dropped? (new_perms =
old_perms & requested_perms) . It's probably useful to throw an error code
if new permissions are requested.
(3) It looks like the code for snd_pcm_anonymous_dup in the patchset takes
a *perm* argument from user-space and discards it.  Is this intended?
(4) I'm not familiar with the lifecycle of all of the objects, and
introducing a custom *dup* routine might cause unexpected problems (e.g.
use-after-free, double-free).  I'm not well-versed-enough in how the
driver-specific stuff is handled underneath e.g. snd_card_file_add /
snd_card_file_remove
to be sure about it.

Zach Riggle |  Android Security |  riggle@google.com |  Austin, TX 🇨🇱


On Thu, Jan 31, 2019 at 1:36 PM Phil Burk <philburk@google.com> wrote:

> Mark and Zach and I talked.
>
> Zach said that "dmabuf" is not a hard requirement. Another "anon_inode"
> would probably be OK as long as the app cannot turn on any permissions
> besides PCM access. Our security team will just need to review the changes.
>
> So I think you should proceed with the "anon_inode:snd-pcm" if you think
> that will be more secure. Thanks for proposing this.
>
> Zach has some notes in his initial review of Jaroslav's code. Zach?
>
> One thing Zach mentioned is that the API should only allow *removing*
> permissions and not adding permissions.
>
> What permissions would be set on the FD given to the app?
>
> Also Mark mentioned that the FD app would have PCM access and "close"
> permission. What flag allows close? What else is permitted under that flag?
> Or is close permission  just a generic "FD" permission unrelated to ALSA?
>
> Thanks for all your work on this. Sorry if I caused alarm. I just wanted
> to make sure we could use the solution you provide.
>
> Phil Burk
>
>
>
>
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

On Thu, 31 Jan 2019 20:54:33 +0100,
Zach Riggle 🖖 wrote:
> 
> The big concerns on our end are:
> 
> (1) Can the buffer be mremap()ed with a different offset into the buffer? 
> This was a concern in the past and the reason for the anon_inode stuff at
> all.  I believe that as long as the *size* of the mapping doesn't change,
> Linux mm will gladly permit mremap() without informing the driver.

Could you elaborate which perspective of mremap() can be a big
problem?  The driver interface does nothing but a standard mmap for
now.

> (2) Can we ensure that permissions can only ever be dropped? (new_perms =
> old_perms & requested_perms) . It's probably useful to throw an error code if
> new permissions are requested.

The permission is bound with each fd, and determined only at creation
time, and unchangeable.

Also, the patches Jaroslav posted can become even simpler / safer;
i.e. we don't need to introduce the perms bits as a start.  IMO, we
can begin with the minimum, mmap-only file ops.  The API should be
defined properly from the beginning (e.g. passing perms argument,
etc), of course.  Then, if any request comes up, we may extend later.

> (3) It looks like the code for snd_pcm_anonymous_dup in the patchset takes a 
> perm argument from user-space and discards it.  Is this intended?

My understanding is that it's the design to be simple.  But we don't
need to stick with it, if you can suggest any better interface.

> (4) I'm not familiar with the lifecycle of all of the objects, and introducing
> a custom dup routine might cause unexpected problems (e.g. use-after-free,
> double-free).  I'm not well-versed-enough in how the driver-specific stuff is
> handled underneath e.g. snd_card_file_add / snd_card_file_remove to be sure
> about it.

That's the advantage of anon-dup implementation; the PCM stream
handling with O_APPEND has been heavily used by alsa-lib dmix PCM
implementations.  So it already survives over decades.


thanks,

Takashi

> Zach Riggle |  Android Security |  riggle@google.com |  Austin, TX 🇨🇱
> 
> On Thu, Jan 31, 2019 at 1:36 PM Phil Burk <philburk@google.com> wrote:
> 
>     Mark and Zach and I talked.
>    
>     Zach said that "dmabuf" is not a hard requirement. Another "anon_inode"
>     would probably be OK as long as the app cannot turn on any permissions
>     besides PCM access. Our security team will just need to review the
>     changes.
>    
>     So I think you should proceed with the "anon_inode:snd-pcm" if you think
>     that will be more secure. Thanks for proposing this.
>    
>     Zach has some notes in his initial review of Jaroslav's code. Zach?
>    
>     One thing Zach mentioned is that the API should only allow removing
>     permissions and not adding permissions.
>    
>     What permissions would be set on the FD given to the app?
>    
>     Also Mark mentioned that the FD app would have PCM access and "close"
>     permission. What flag allows close? What else is permitted under that
>     flag? Or is close permission  just a generic "FD" permission unrelated to
>     ALSA?
>    
>     Thanks for all your work on this. Sorry if I caused alarm. I just wanted
>     to make sure we could use the solution you provide.
>    
>     Phil Burk
> 
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Jaroslav Kysela]

Dne 31.1.2019 v 21:32 Takashi Iwai napsal(a):
> On Thu, 31 Jan 2019 20:54:33 +0100,
> Zach Riggle 🖖 wrote:
>>
>> The big concerns on our end are:
>>
>> (1) Can the buffer be mremap()ed with a different offset into the buffer? 
>> This was a concern in the past and the reason for the anon_inode stuff at
>> all.  I believe that as long as the *size* of the mapping doesn't change,
>> Linux mm will gladly permit mremap() without informing the driver.
> 
> Could you elaborate which perspective of mremap() can be a big
> problem?  The driver interface does nothing but a standard mmap for
> now.

Yes, basically, the sound buffer (including it's size) is locked in the
kernel space until the last mmaped area is active as it should be. See
substream->mmap_count . The mmap implementation is similar what the
dma-buf file interface does.

>> (2) Can we ensure that permissions can only ever be dropped? (new_perms =
>> old_perms & requested_perms) . It's probably useful to throw an error code if
>> new permissions are requested.
> 
> The permission is bound with each fd, and determined only at creation
> time, and unchangeable.

Exactly. The caller who creates the duplicated file descriptor should
specify the permissions which are locked for the lifetime of the
restricted file descriptor.

> Also, the patches Jaroslav posted can become even simpler / safer;
> i.e. we don't need to introduce the perms bits as a start.  IMO, we
> can begin with the minimum, mmap-only file ops.  The API should be
> defined properly from the beginning (e.g. passing perms argument,
> etc), of course.  Then, if any request comes up, we may extend later.

I agree. We can have just two modes for the beginning:

a) full one (useful for testing)
b) buffer only (allow just sound data mmap)

The question, if we should use different names (line anon_inode:snd-pcm
and anon_inode:snd-pcm-buffer) for the anonymous inodes remains. I
believe it might be good to distinguish this to allow the proper SELinux
audit.

>> (3) It looks like the code for snd_pcm_anonymous_dup in the patchset takes a 
>> perm argument from user-space and discards it.  Is this intended?
> 
> My understanding is that it's the design to be simple.  But we don't
> need to stick with it, if you can suggest any better interface.

Yes, I preferred the simplicity (so only one integer argument in and out).

>> (4) I'm not familiar with the lifecycle of all of the objects, and introducing
>> a custom dup routine might cause unexpected problems (e.g. use-after-free,
>> double-free).  I'm not well-versed-enough in how the driver-specific stuff is
>> handled underneath e.g. snd_card_file_add / snd_card_file_remove to be sure
>> about it.
> 
> That's the advantage of anon-dup implementation; the PCM stream
> handling with O_APPEND has been heavily used by alsa-lib dmix PCM
> implementations.  So it already survives over decades.

Yes, there is simple reference counter which contains the count of the
file decriptors assigned to the pcm substream -
substream->ref_count . See snd_pcm_release_substream() for more details.

>> On Thu, Jan 31, 2019 at 1:36 PM Phil Burk <philburk@google.com> wrote:
>>>     permission. What flag allows close? What else is permitted under that
>>     flag? Or is close permission  just a generic "FD" permission unrelated to
>>     ALSA?

All file descriptors must have defined the 'release' callback in the
kernel to free the resources when the close syscall is executed.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 834 bytes --]

On Thu, Jan 31, 2019 at 09:32:27PM +0100, Takashi Iwai wrote:
> On Thu, 31 Jan 2019 20:54:33 +0100,
> Zach Riggle 🖖 wrote:

> > (1) Can the buffer be mremap()ed with a different offset into the buffer? 
> > This was a concern in the past and the reason for the anon_inode stuff at
> > all.  I believe that as long as the *size* of the mapping doesn't change,
> > Linux mm will gladly permit mremap() without informing the driver.

> Could you elaborate which perspective of mremap() can be a big
> problem?  The driver interface does nothing but a standard mmap for
> now.

I believe the issue was that if someone could remap the buffer to gain
access to memory outside the memory allocated for the PCM buffer that
would be a problem.  To be honest I'm surprised that this might be a
general issue with mmap().

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 545 bytes --]

On Fri, Feb 01, 2019 at 10:55:24AM +0100, Jaroslav Kysela wrote:

> I agree. We can have just two modes for the beginning:

> a) full one (useful for testing)
> b) buffer only (allow just sound data mmap)

> The question, if we should use different names (line anon_inode:snd-pcm
> and anon_inode:snd-pcm-buffer) for the anonymous inodes remains. I
> believe it might be good to distinguish this to allow the proper SELinux
> audit.

I agree that the separte names seems better, it gives more flexibility
and control to people writing policies.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Phil Burk]

[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]

Thank you all for sorting this out. It seems like we are moving in a really
good direction.

> I agree. We can have just two modes for the beginning:
> a) full one (useful for testing)
> b) buffer only (allow just sound data mmap)

The full one is would also be used by the HAL for querying the DSP position.

>  if we should use different names (like anon_inode:snd-pcm and
anon_inode:snd-pcm-buffer)

That would be helpful.

I have attached a revised requirements doc. The original doc was more of a
HowTo for OEMs to create the "anon_inode:dmabuf" FD.. This clarifies the
requirements and allows for the use of "anon_inode:snd-pcm*". It should
match what we have arrived at by discussion. Let me know if it makes sense.

Thanks,
Phil Burk


On Fri, Feb 1, 2019 at 5:01 AM Mark Brown <broonie@kernel.org> wrote:

> On Fri, Feb 01, 2019 at 10:55:24AM +0100, Jaroslav Kysela wrote:
>
> > I agree. We can have just two modes for the beginning:
>
> > a) full one (useful for testing)
> > b) buffer only (allow just sound data mmap)
>
> > The question, if we should use different names (line anon_inode:snd-pcm
> > and anon_inode:snd-pcm-buffer) for the anonymous inodes remains. I
> > believe it might be good to distinguish this to allow the proper SELinux
> > audit.
>
> I agree that the separte names seems better, it gives more flexibility
> and control to people writing policies.
>

[-- Attachment #2: RequirementsSecureMMapFileDescriptor.pdf --]
[-- Type: application/pdf, Size: 31320 bytes --]

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

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

[Jaroslav Kysela]

Dne 1.2.2019 v 16:31 Phil Burk napsal(a):
> Thank you all for sorting this out. It seems like we are moving in a
> really good direction.
> 
>> I agree. We can have just two modes for the beginning:
>> a) full one (useful for testing)
>> b) buffer only (allow just sound data mmap)
> 
> The full one is would also be used by the HAL for querying the DSP position.
> 
>>  if we should use different names (like anon_inode:snd-pcm and
> anon_inode:snd-pcm-buffer) 
> 
> That would be helpful.
> 
> I have attached a revised requirements doc. The original doc was more of
> a HowTo for OEMs to create the "anon_inode:dmabuf" FD.. This clarifies
> the requirements and allows for the use of "anon_inode:snd-pcm*". It
> should match what we have arrived at by discussion. Let me know if it
> makes sense.

It looks fine, but the HAL will use probably the standard sound device
open (/dev/snd/), doesn't? So:

fd1 - /dev/snd/pcm (HAL) - standard sound device inode (no restrictions)
fd2 - anon_inode:snd-pcm-buffer (for the EXCLUSIVE access)

With modes, I talked about the anonymous dup ioctl parameter. If there's
another resource manager above HAL, the situation might be:

fd1 - /dev/snd/pcm (resource manager) - standard sound device inode
fd2 - anon_inode:snd-pcm (access to the full pcm sound API using the
anonymous inode)
fd3 - anon_inode:snd-pcm-buffer (for the EXCLUSIVE access)

Perhaps you have different layers in Android.

			Thanks,
				Jaroslav


-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Phil Burk]

Thanks for the clarification. I was thinking the anon_inode:snd-pcm FD
replaced the /dev/snd/pcm FD.

> fd1 - /dev/snd/pcm (HAL) - standard sound device inode (no restrictions)
> fd2 - anon_inode:snd-pcm-buffer (for the EXCLUSIVE access)

That sounds right.

Thanks again for this work. ALSA is very important for Android Audio.

Phil Burk


On Fri, Feb 1, 2019 at 8:28 AM Jaroslav Kysela <perex@perex.cz> wrote:

> Dne 1.2.2019 v 16:31 Phil Burk napsal(a):
> > Thank you all for sorting this out. It seems like we are moving in a
> > really good direction.
> >
> >> I agree. We can have just two modes for the beginning:
> >> a) full one (useful for testing)
> >> b) buffer only (allow just sound data mmap)
> >
> > The full one is would also be used by the HAL for querying the DSP
> position.
> >
> >>  if we should use different names (like anon_inode:snd-pcm and
> > anon_inode:snd-pcm-buffer)
> >
> > That would be helpful.
> >
> > I have attached a revised requirements doc. The original doc was more of
> > a HowTo for OEMs to create the "anon_inode:dmabuf" FD.. This clarifies
> > the requirements and allows for the use of "anon_inode:snd-pcm*". It
> > should match what we have arrived at by discussion. Let me know if it
> > makes sense.
>
> It looks fine, but the HAL will use probably the standard sound device
> open (/dev/snd/), doesn't? So:
>
> fd1 - /dev/snd/pcm (HAL) - standard sound device inode (no restrictions)
> fd2 - anon_inode:snd-pcm-buffer (for the EXCLUSIVE access)
>
> With modes, I talked about the anonymous dup ioctl parameter. If there's
> another resource manager above HAL, the situation might be:
>
> fd1 - /dev/snd/pcm (resource manager) - standard sound device inode
> fd2 - anon_inode:snd-pcm (access to the full pcm sound API using the
> anonymous inode)
> fd3 - anon_inode:snd-pcm-buffer (for the EXCLUSIVE access)
>
> Perhaps you have different layers in Android.
>
>                         Thanks,
>                                 Jaroslav
>
>
> --
> Jaroslav Kysela <perex@perex.cz>
> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
>