mmc: Add ioctl to let userspace apps send ACMDs

mmc: Add ioctl to let userspace apps send ACMDs

[John Calixto]

Since this patch has gone through multiple revisions now, I thought I'd
list the changes in each revision:

- v4
    - replace postsleep udelay() with usleep_range()
    - add cmd_timeout_ms field for R1B commands

- v3
    - copy data from userspace before claiming host
    - break out copy from userspace into its own function
    - verify that caller has CAP_SYS_RAWIO
    - rename ``struct sd_ioc_cmd`` to ``struct mmc_ioc_cmd`` because it
      applies generally, not just to SD
    - make struct mmc_ioc_cmd the same between 32-bit and 64-bit to
      simplify compat_ioctl()
    - export include/linux/mmc/ioctl.h when you ``make headers_install``

- v2
    - make initialization of struct declarations match kernel style
    - only allow ioctl() on whole block device, not partition
    - remove extraneous printks
    - implement compat_ioctl()
    - remove version field from ``struct sd_ioc_cmd``

John

[PATCH v4] mmc: Add ioctl to let userspace apps send ACMDs

[John Calixto]

Sending ACMDs from userspace is useful for such things as:

    - The security application of an SD card (SD Specification, Part 3,
      Security)

    - SD passthrough for virtual machines

Tested on TI PCIxx12 (SDHCI), Sigma Designs SMP8652 SoC, TI OMAP3621
SoC, TI OMAP3630 SoC, Samsung S5PC110 SoC, Qualcomm MSM7200A SoC.

Signed-off-by: John Calixto <john.calixto@modsystems.com>
Reviewed-by: Andrei Warkentin <andreiw@motorola.com>
---
 drivers/mmc/card/block.c  |  196 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/core/sd_ops.c |    3 +-
 include/linux/Kbuild      |    1 +
 include/linux/mmc/Kbuild  |    1 +
 include/linux/mmc/core.h  |    1 +
 include/linux/mmc/ioctl.h |   32 +++++++
 6 files changed, 233 insertions(+), 1 deletions(-)
 create mode 100644 include/linux/mmc/Kbuild
 create mode 100644 include/linux/mmc/ioctl.h

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 61d233a..5e09801 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -31,7 +31,10 @@
 #include <linux/mutex.h>
 #include <linux/scatterlist.h>
 #include <linux/string_helpers.h>
+#include <linux/delay.h>
+#include <linux/capability.h>
 
+#include <linux/mmc/ioctl.h>
 #include <linux/mmc/card.h>
 #include <linux/mmc/host.h>
 #include <linux/mmc/mmc.h>
@@ -158,11 +161,204 @@ mmc_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 	return 0;
 }
 
+struct mmc_blk_ioc_data {
+	struct mmc_ioc_cmd ic;
+	unsigned char *buf;
+	size_t buf_bytes;
+};
+
+static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user(
+	struct mmc_ioc_cmd __user *user)
+{
+	struct mmc_blk_ioc_data *idata;
+	int err;
+
+	idata = kzalloc(sizeof(*idata), GFP_KERNEL);
+	if (!idata) {
+		err = -ENOMEM;
+		goto copy_err;
+	}
+
+	if (copy_from_user(&idata->ic, user, sizeof(idata->ic))) {
+		err = -EFAULT;
+		goto copy_err;
+	}
+
+	idata->buf_bytes = idata->ic.blksz * idata->ic.blocks;
+
+	idata->buf = kzalloc(idata->buf_bytes, GFP_KERNEL);
+	if (!idata->buf) {
+		err = -ENOMEM;
+		goto copy_err;
+	}
+
+	if (copy_from_user(idata->buf, (u8 *) user->data_ptr, idata->buf_bytes)) {
+		err = -EFAULT;
+		goto copy_err;
+	}
+
+	return idata;
+
+copy_err:
+	kfree(idata->buf);
+	kfree(idata);
+	return ERR_PTR(err);
+
+}
+
+static int mmc_blk_ioctl_acmd(struct block_device *bdev,
+	struct mmc_ioc_cmd __user *ic_ptr)
+{
+	struct mmc_blk_ioc_data *idata;
+	struct mmc_blk_data *md;
+	struct mmc_card *card;
+	struct mmc_command cmd = {0};
+	struct mmc_data data = {0};
+	struct mmc_request mrq = {0};
+	struct scatterlist sg;
+	int err;
+
+	/*
+	 * The caller must have CAP_SYS_RAWIO, and must be calling this on the
+	 * whole block device, not on a partition.  This prevents overspray
+	 * between sibling partitions.
+	 */
+	if ((!capable(CAP_SYS_RAWIO)) || (bdev != bdev->bd_contains))
+		return -EPERM;
+
+	idata = mmc_blk_ioctl_copy_from_user(ic_ptr);
+	if (IS_ERR(idata))
+		return PTR_ERR(idata);
+
+	cmd.opcode = idata->ic.opcode;
+	cmd.arg = idata->ic.arg;
+	cmd.flags = idata->ic.flags;
+
+	data.sg = &sg;
+	data.sg_len = 1;
+	data.blksz = idata->ic.blksz;
+	data.blocks = idata->ic.blocks;
+
+	sg_init_one(data.sg, idata->buf, idata->buf_bytes);
+
+	if (idata->ic.write_flag)
+		data.flags = MMC_DATA_WRITE;
+	else
+		data.flags = MMC_DATA_READ;
+
+	mrq.cmd = &cmd;
+	mrq.data = &data;
+
+	md = mmc_blk_get(bdev->bd_disk);
+	if (!md) {
+		err = -EINVAL;
+		goto acmd_done;
+	}
+
+	card = md->queue.card;
+	if (IS_ERR(card)) {
+		err = PTR_ERR(card);
+		goto acmd_done;
+	}
+
+	mmc_claim_host(card->host);
+
+	err = mmc_app_cmd(card->host, card);
+	if (err)
+		goto acmd_rel_host;
+
+	/* data.flags must already be set before doing this. */
+	mmc_set_data_timeout(&data, card);
+	/* Allow overriding the timeout_ns for empirical tuning. */
+	if (idata->ic.data_timeout_ns)
+		data.timeout_ns = idata->ic.data_timeout_ns;
+
+	if ((cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B) {
+		/*
+		 * Pretend this is a data transfer and rely on the host driver
+		 * to compute timeout.  When all host drivers support
+		 * cmd.cmd_timeout for R1B, this can be changed to:
+		 *
+		 *     mrq.data = NULL;
+		 *     cmd.cmd_timeout = idata->ic.cmd_timeout_ms;
+		 */
+		data.timeout_ns = idata->ic.cmd_timeout_ms * 1000000;
+	}
+
+	mmc_wait_for_req(card->host, &mrq);
+
+	if (cmd.error) {
+		dev_err(mmc_dev(card->host), "%s: cmd error %d\n",
+						__func__, cmd.error);
+		err = cmd.error;
+		goto acmd_rel_host;
+	}
+	if (data.error) {
+		dev_err(mmc_dev(card->host), "%s: data error %d\n",
+						__func__, data.error);
+		err = data.error;
+		goto acmd_rel_host;
+	}
+
+	/*
+	 * According to the SD specs, some commands require a delay after
+	 * issuing the command.
+	 */
+	if (idata->ic.postsleep_min_us)
+		usleep_range(idata->ic.postsleep_min_us, idata->ic.postsleep_max_us);
+
+	if (copy_to_user(&(ic_ptr->response), cmd.resp, sizeof(cmd.resp))) {
+		err = -EFAULT;
+		goto acmd_rel_host;
+	}
+
+	if (!idata->ic.write_flag) {
+		if (copy_to_user((u8 *) ic_ptr->data_ptr, idata->buf, idata->buf_bytes)) {
+			err = -EFAULT;
+			goto acmd_rel_host;
+		}
+	}
+
+acmd_rel_host:
+	mmc_release_host(card->host);
+
+acmd_done:
+	mmc_blk_put(md);
+	kfree(idata->buf);
+	kfree(idata);
+	return err;
+}
+
+static int mmc_blk_ioctl(struct block_device *bdev, fmode_t mode,
+	unsigned int cmd, unsigned long arg)
+{
+	int ret = -EINVAL;
+	mutex_lock(&block_mutex);
+	if (cmd == MMC_IOC_ACMD)
+		ret = mmc_blk_ioctl_acmd(bdev, (struct mmc_ioc_cmd __user *)arg);
+	mutex_unlock(&block_mutex);
+	return ret;
+}
+
+#ifdef CONFIG_COMPAT
+static int mmc_blk_compat_ioctl(struct block_device *bdev, fmode_t mode,
+	unsigned int cmd, unsigned long arg)
+{
+	struct mmc_ioc_cmd __user *ic = (struct mmc_ioc_cmd __user *) arg;
+	ic->data_ptr = ic->data_ptr & 0xffffffff;
+	return mmc_blk_ioctl(bdev, mode, cmd, arg);
+}
+#endif
+
 static const struct block_device_operations mmc_bdops = {
 	.open			= mmc_blk_open,
 	.release		= mmc_blk_release,
 	.getgeo			= mmc_blk_getgeo,
 	.owner			= THIS_MODULE,
+	.ioctl			= mmc_blk_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl		= mmc_blk_compat_ioctl,
+#endif
 };
 
 struct mmc_blk_request {
diff --git a/drivers/mmc/core/sd_ops.c b/drivers/mmc/core/sd_ops.c
index 797cdb5..990dd43 100644
--- a/drivers/mmc/core/sd_ops.c
+++ b/drivers/mmc/core/sd_ops.c
@@ -20,7 +20,7 @@
 #include "core.h"
 #include "sd_ops.h"
 
-static int mmc_app_cmd(struct mmc_host *host, struct mmc_card *card)
+int mmc_app_cmd(struct mmc_host *host, struct mmc_card *card)
 {
 	int err;
 	struct mmc_command cmd;
@@ -48,6 +48,7 @@ static int mmc_app_cmd(struct mmc_host *host, struct mmc_card *card)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(mmc_app_cmd);
 
 /**
  *	mmc_wait_for_app_cmd - start an application command and wait for
diff --git a/include/linux/Kbuild b/include/linux/Kbuild
index 75cf611..ed38527 100644
--- a/include/linux/Kbuild
+++ b/include/linux/Kbuild
@@ -4,6 +4,7 @@ header-y += caif/
 header-y += dvb/
 header-y += hdlc/
 header-y += isdn/
+header-y += mmc/
 header-y += nfsd/
 header-y += raid/
 header-y += spi/
diff --git a/include/linux/mmc/Kbuild b/include/linux/mmc/Kbuild
new file mode 100644
index 0000000..1fb2644
--- /dev/null
+++ b/include/linux/mmc/Kbuild
@@ -0,0 +1 @@
+header-y += ioctl.h
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 07f27af..bfc6127 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -133,6 +133,7 @@ struct mmc_card;
 
 extern void mmc_wait_for_req(struct mmc_host *, struct mmc_request *);
 extern int mmc_wait_for_cmd(struct mmc_host *, struct mmc_command *, int);
+extern int mmc_app_cmd(struct mmc_host *, struct mmc_card *);
 extern int mmc_wait_for_app_cmd(struct mmc_host *, struct mmc_card *,
 	struct mmc_command *, int);
 
diff --git a/include/linux/mmc/ioctl.h b/include/linux/mmc/ioctl.h
new file mode 100644
index 0000000..d0f11fb
--- /dev/null
+++ b/include/linux/mmc/ioctl.h
@@ -0,0 +1,32 @@
+#ifndef _MMC_IOCTL_H
+#define _MMC_IOCTL_H
+struct mmc_ioc_cmd {
+	/* implies direction of data.  true = write, false = read */
+	int write_flag;
+
+	__u32 opcode;
+	__u32 arg;
+	__u32 response[4];  /* CMD response */
+	unsigned int flags;
+	unsigned int blksz;
+	unsigned int blocks;
+
+	/*
+	 * Sleep at least postsleep_min_us useconds, and at most
+	 * postsleep_max_us useconds *after* issuing command.  Needed for some
+	 * read commands for which cards have no other way of indicating
+	 * they're ready for the next command (i.e. there is no equivalent of a
+	 * "busy" indicator for read operations).
+	 */
+	unsigned int postsleep_min_us;
+	unsigned int postsleep_max_us;
+
+	/*
+	 * Override driver-computed timeouts.  Note the difference in units!
+	 */
+	unsigned int data_timeout_ns;
+	unsigned int cmd_timeout_ms;
+	__u64 data_ptr;  /* DAT buffer */
+};
+#define MMC_IOC_ACMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)
+#endif  /* _MMC_IOCTL_H */
-- 
1.7.4.1

Re: [PATCH v4] mmc: Add ioctl to let userspace apps send ACMDs

[Michał Mirosław]

2011/4/11 John Calixto <john.calixto@modsystems.com>:
[...]
> --- /dev/null
> +++ b/include/linux/mmc/ioctl.h
> @@ -0,0 +1,32 @@
> +#ifndef _MMC_IOCTL_H
> +#define _MMC_IOCTL_H
> +struct mmc_ioc_cmd {
> +       /* implies direction of data.  true = write, false = read */
> +       int write_flag;
> +
> +       __u32 opcode;
> +       __u32 arg;
> +       __u32 response[4];  /* CMD response */
> +       unsigned int flags;
> +       unsigned int blksz;
> +       unsigned int blocks;
> +
> +       /*
> +        * Sleep at least postsleep_min_us useconds, and at most
> +        * postsleep_max_us useconds *after* issuing command.  Needed for some
> +        * read commands for which cards have no other way of indicating
> +        * they're ready for the next command (i.e. there is no equivalent of a
> +        * "busy" indicator for read operations).
> +        */
> +       unsigned int postsleep_min_us;
> +       unsigned int postsleep_max_us;
> +
> +       /*
> +        * Override driver-computed timeouts.  Note the difference in units!
> +        */
> +       unsigned int data_timeout_ns;
> +       unsigned int cmd_timeout_ms;
> +       __u64 data_ptr;  /* DAT buffer */

This will be more natural if you have an anonymous union here:
union {
  __u64 data_ptr_l;
  void *data_ptr;
};

> +};
> +#define MMC_IOC_ACMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)
> +#endif  /* _MMC_IOCTL_H */

Best Regards,
Michał Mirosław

Re: [PATCH v4] mmc: Add ioctl to let userspace apps send ACMDs

[John Calixto]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1509 bytes --]

On Tue, 12 Apr 2011, Michał Mirosław wrote:
> 2011/4/11 John Calixto <john.calixto@modsystems.com>:
> [...]
> > --- /dev/null
> > +++ b/include/linux/mmc/ioctl.h
> > @@ -0,0 +1,32 @@
> > +#ifndef _MMC_IOCTL_H
> > +#define _MMC_IOCTL_H
> > +struct mmc_ioc_cmd {
> > +       /* implies direction of data.  true = write, false = read */
> > +       int write_flag;
> > +
> > +       __u32 opcode;
> > +       __u32 arg;
> > +       __u32 response[4];  /* CMD response */
> > +       unsigned int flags;
> > +       unsigned int blksz;
> > +       unsigned int blocks;
> > +
> > +       /*
> > +        * Sleep at least postsleep_min_us useconds, and at most
> > +        * postsleep_max_us useconds *after* issuing command.  Needed for some
> > +        * read commands for which cards have no other way of indicating
> > +        * they're ready for the next command (i.e. there is no equivalent of a
> > +        * "busy" indicator for read operations).
> > +        */
> > +       unsigned int postsleep_min_us;
> > +       unsigned int postsleep_max_us;
> > +
> > +       /*
> > +        * Override driver-computed timeouts.  Note the difference in units!
> > +        */
> > +       unsigned int data_timeout_ns;
> > +       unsigned int cmd_timeout_ms;
> > +       __u64 data_ptr;  /* DAT buffer */
> 
> This will be more natural if you have an anonymous union here:
> union {
>   __u64 data_ptr_l;
>   void *data_ptr;
> };
> 

Ah, but of course!  Thanks!

John

Re: [PATCH v4] mmc: Add ioctl to let userspace apps send ACMDs

[Arnd Bergmann]

On Tuesday 12 April 2011, Michał Mirosław wrote:
> > +       unsigned int cmd_timeout_ms;
> > +       __u64 data_ptr;  /* DAT buffer */
> 
> This will be more natural if you have an anonymous union here:
> union {
>   __u64 data_ptr_
>  void *data_ptr;
> };

No, that really does not work. It's important for all members of the ioctl data
structure to have a fixed size, independent of the size of long or pointer.

If you do a union, the pointer ends up in the first 32 bits of the 64 bit member,
which does not work on big-endian architectures. It also doesn't work on 31
bit architectures, although that is a minor worry here.

	Arnd

Re: [PATCH v4] mmc: Add ioctl to let userspace apps send ACMDs

[John Calixto]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1038 bytes --]

On Wed, 13 Apr 2011, Arnd Bergmann wrote:
> On Tuesday 12 April 2011, Michał Mirosław wrote:
> > > +       unsigned int cmd_timeout_ms;
> > > +       __u64 data_ptr;  /* DAT buffer */
> > 
> > This will be more natural if you have an anonymous union here:
> > union {
> >   __u64 data_ptr_
> >  void *data_ptr;
> > };
> 
> No, that really does not work. It's important for all members of the ioctl data
> structure to have a fixed size, independent of the size of long or pointer.
> 
> If you do a union, the pointer ends up in the first 32 bits of the 64 bit member,
> which does not work on big-endian architectures. It also doesn't work on 31
> bit architectures, although that is a minor worry here.
> 

Gah!  OK, so much for the v5 I just sent then.  What do you think about
the compat_ioctl that I sent in v2?  It means having the extra 32-bit
compat structure, but at least all the compat overhead is conditional
upon CONFIG_COMPAT.  If you're not using CONFIG_COMPAT, you don't get
any compat cruft.

John

Re: [PATCH v4] mmc: Add ioctl to let userspace apps send ACMDs

[Arnd Bergmann]

On Monday 11 April 2011, John Calixto wrote:
> Sending ACMDs from userspace is useful for such things as:
> 
>     - The security application of an SD card (SD Specification, Part 3,
>       Security)
> 
>     - SD passthrough for virtual machines
> 
> Tested on TI PCIxx12 (SDHCI), Sigma Designs SMP8652 SoC, TI OMAP3621
> SoC, TI OMAP3630 SoC, Samsung S5PC110 SoC, Qualcomm MSM7200A SoC.
> 
> Signed-off-by: John Calixto <john.calixto@modsystems.com>
> Reviewed-by: Andrei Warkentin <andreiw@motorola.com>

Since the code is limited to ACMD and cannot do arbitrary commands, it's actually
not possible to use this for the passthrough scenario, so you should not mention
it in the changelog.

I would also still advocate something more high-level here because it's limited
to a single use case. If you make the ioctl interface do the security commands
directly, you would not need to rely on CAP_SYS_RAWIO.

> +static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user(
> +	struct mmc_ioc_cmd __user *user)
> +{
> +	struct mmc_blk_ioc_data *idata;
> +	int err;
> +
> +	idata = kzalloc(sizeof(*idata), GFP_KERNEL);
> +	if (!idata) {
> +		err = -ENOMEM;
> +		goto copy_err;
> +	}
> +
> +	if (copy_from_user(&idata->ic, user, sizeof(idata->ic))) {
> +		err = -EFAULT;
> +		goto copy_err;
> +	}
> +
> +	idata->buf_bytes = idata->ic.blksz * idata->ic.blocks;
> +
> +	idata->buf = kzalloc(idata->buf_bytes, GFP_KERNEL);

You should probably check the size and allow a fixed maximum here.

> +static int mmc_blk_ioctl(struct block_device *bdev, fmode_t mode,
> +	unsigned int cmd, unsigned long arg)
> +{
> +	int ret = -EINVAL;
> +	mutex_lock(&block_mutex);
> +	if (cmd == MMC_IOC_ACMD)
> +		ret = mmc_blk_ioctl_acmd(bdev, (struct mmc_ioc_cmd __user *)arg);
> +	mutex_unlock(&block_mutex);
> +	return ret;
> +}

Why do you need the mutex here?

> +#ifdef CONFIG_COMPAT
> +static int mmc_blk_compat_ioctl(struct block_device *bdev, fmode_t mode,
> +	unsigned int cmd, unsigned long arg)
> +{
> +	struct mmc_ioc_cmd __user *ic = (struct mmc_ioc_cmd __user *) arg;
> +	ic->data_ptr = ic->data_ptr & 0xffffffff;

This conversion should use

	ptr = (unsigned long)compat_ptr(ptr32);

You must never access __user pointers with a direct pointer dereference, but have
to use copy_{from,to}_user or {get,put}_user. The code you have here allows a
fairly simple root exploit.

	Arnd

Re: [PATCH v4] mmc: Add ioctl to let userspace apps send ACMDs

[John Calixto]

Hi Arnd,

On Wed, 13 Apr 2011, Arnd Bergmann wrote:
> On Monday 11 April 2011, John Calixto wrote:
> > Sending ACMDs from userspace is useful for such things as:
> > 
> >     - The security application of an SD card (SD Specification, Part 3,
> >       Security)
> > 
> >     - SD passthrough for virtual machines
> > 
> > Tested on TI PCIxx12 (SDHCI), Sigma Designs SMP8652 SoC, TI OMAP3621
> > SoC, TI OMAP3630 SoC, Samsung S5PC110 SoC, Qualcomm MSM7200A SoC.
> > 
> > Signed-off-by: John Calixto <john.calixto@modsystems.com>
> > Reviewed-by: Andrei Warkentin <andreiw@motorola.com>
> 
> Since the code is limited to ACMD and cannot do arbitrary commands, it's actually
> not possible to use this for the passthrough scenario, so you should not mention
> it in the changelog.
> 
> I would also still advocate something more high-level here because it's limited
> to a single use case. If you make the ioctl interface do the security commands
> directly, you would not need to rely on CAP_SYS_RAWIO.
> 

I'm happy to remove the text about passthrough from the changelog, but
it is a valid use for this ioctl.  I agree that ACMD by itself is not
sufficient for full passthrough, but this patch is a starting point for
anyone wanting to implement full CMD passthrough.

There are also several ACMD opcodes that are not related to security,
but to functionality like requesting to change the signalling voltage,
setting bus width, setting pre-erase block count, etc...  I think those
commands are what caused others to request some kind of capability
restriction.

> > +static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user(
> > +	struct mmc_ioc_cmd __user *user)
> > +{
> > +	struct mmc_blk_ioc_data *idata;
> > +	int err;
> > +
> > +	idata = kzalloc(sizeof(*idata), GFP_KERNEL);
> > +	if (!idata) {
> > +		err = -ENOMEM;
> > +		goto copy_err;
> > +	}
> > +
> > +	if (copy_from_user(&idata->ic, user, sizeof(idata->ic))) {
> > +		err = -EFAULT;
> > +		goto copy_err;
> > +	}
> > +
> > +	idata->buf_bytes = idata->ic.blksz * idata->ic.blocks;
> > +
> > +	idata->buf = kzalloc(idata->buf_bytes, GFP_KERNEL);
> 
> You should probably check the size and allow a fixed maximum here.
> 

OK, I'll read the specs to find a suitable maximum.

> > +static int mmc_blk_ioctl(struct block_device *bdev, fmode_t mode,
> > +	unsigned int cmd, unsigned long arg)
> > +{
> > +	int ret = -EINVAL;
> > +	mutex_lock(&block_mutex);
> > +	if (cmd == MMC_IOC_ACMD)
> > +		ret = mmc_blk_ioctl_acmd(bdev, (struct mmc_ioc_cmd __user *)arg);
> > +	mutex_unlock(&block_mutex);
> > +	return ret;
> > +}
> 
> Why do you need the mutex here?
> 

This patch originated an a much earlier version of the kernel that did
not have unlocked ioctls.  To be honest, I based this on what I found in
other ioctl implementations.  Looking at who else grabs block_mutex, I
think it's safe to remove this.

> > +#ifdef CONFIG_COMPAT
> > +static int mmc_blk_compat_ioctl(struct block_device *bdev, fmode_t mode,
> > +	unsigned int cmd, unsigned long arg)
> > +{
> > +	struct mmc_ioc_cmd __user *ic = (struct mmc_ioc_cmd __user *) arg;
> > +	ic->data_ptr = ic->data_ptr & 0xffffffff;
> 
> This conversion should use
> 
> 	ptr = (unsigned long)compat_ptr(ptr32);
> 
> You must never access __user pointers with a direct pointer dereference, but have
> to use copy_{from,to}_user or {get,put}_user. The code you have here allows a
> fairly simple root exploit.

Got it - thanks!

John

Re: [PATCH v4] mmc: Add ioctl to let userspace apps send ACMDs

[Arnd Bergmann]

On Wednesday 13 April 2011, John Calixto wrote:
> Gah!  OK, so much for the v5 I just sent then.  What do you think about
> the compat_ioctl that I sent in v2?  It means having the extra 32-bit
> compat structure, but at least all the compat overhead is conditional
> upon CONFIG_COMPAT.  If you're not using CONFIG_COMPAT, you don't get
> any compat cruft.
> 

A single __u64 is sufficient, because the user space will do the correct
conversion from pointer to 64-bit integer then. The only conversion
you need to worry about is the actual pointer to the main structure,
which needs the compat_ptr() magic.

	Arnd

Re: [PATCH v4] mmc: Add ioctl to let userspace apps send ACMDs

[Arnd Bergmann]

On Wednesday 13 April 2011, John Calixto wrote:
> > Since the code is limited to ACMD and cannot do arbitrary commands, it's actually
> > not possible to use this for the passthrough scenario, so you should not mention
> > it in the changelog.
> > 
> > I would also still advocate something more high-level here because it's limited
> > to a single use case. If you make the ioctl interface do the security commands
> > directly, you would not need to rely on CAP_SYS_RAWIO.
> > 
> 
> I'm happy to remove the text about passthrough from the changelog, but
> it is a valid use for this ioctl.  I agree that ACMD by itself is not
> sufficient for full passthrough, but this patch is a starting point for
> anyone wanting to implement full CMD passthrough.
> 
> There are also several ACMD opcodes that are not related to security,
> but to functionality like requesting to change the signalling voltage,
> setting bus width, setting pre-erase block count, etc...  I think those
> commands are what caused others to request some kind of capability
> restriction.
> 

Ok, I see.

In v6, it seems you have implemented the full CMD passthrough, if I
read it correctly. Is there anything still missing?

	Arnd

Re: [PATCH v4] mmc: Add ioctl to let userspace apps send ACMDs

[John Calixto]

On Mon, 18 Apr 2011, Arnd Bergmann wrote:
> On Wednesday 13 April 2011, John Calixto wrote:
> > Gah!  OK, so much for the v5 I just sent then.  What do you think about
> > the compat_ioctl that I sent in v2?  It means having the extra 32-bit
> > compat structure, but at least all the compat overhead is conditional
> > upon CONFIG_COMPAT.  If you're not using CONFIG_COMPAT, you don't get
> > any compat cruft.
> > 
> 
> A single __u64 is sufficient, because the user space will do the correct
> conversion from pointer to 64-bit integer then. The only conversion
> you need to worry about is the actual pointer to the main structure,
> which needs the compat_ptr() magic.
> 

Yep.  Please see the v6 patch for my latest implementation.

John

Re: [PATCH v4] mmc: Add ioctl to let userspace apps send ACMDs

[John Calixto]

On Mon, 18 Apr 2011, Arnd Bergmann wrote:
> In v6, it seems you have implemented the full CMD passthrough, if I
> read it correctly. Is there anything still missing?
> 

Hi Arnd,

As far as I have tested, the v6 patch does indeed have full CMD
passthrough.

John