Re: [PATCH v2 1/2] mmc: Add ioctl to let userspace apps send ACMDs

["Michał Mirosław" <mirqus@gmail.com>]

2011/4/5 John Calixto <john.calixto@modsystems.com>:
> 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>
> ---
>  drivers/mmc/card/block.c  |  191 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/mmc/core/sd_ops.c |    3 +-
>  include/linux/mmc/core.h  |    1 +
>  include/linux/mmc/sd.h    |   16 ++++
>  4 files changed, 210 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 61d233a..c2e107c 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -31,6 +31,8 @@
>  #include <linux/mutex.h>
>  #include <linux/scatterlist.h>
>  #include <linux/string_helpers.h>
> +#include <linux/compat.h>
> +#include <linux/delay.h>
>
>  #include <linux/mmc/card.h>
>  #include <linux/mmc/host.h>
> @@ -158,11 +160,200 @@ mmc_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo)
>        return 0;
>  }
>
> +static int mmc_blk_ioctl_acmd(struct block_device *bdev,
> +       struct sd_ioc_cmd __user *sdic_ptr)
> +{
> +       struct sd_ioc_cmd sdic;
> +       struct mmc_blk_data *md;
> +       struct mmc_host *host;
> +       struct mmc_card *card;
> +       struct mmc_command cmd = {0};
> +       struct mmc_data data = {0};
> +       struct mmc_request mrq = {0};
> +       struct scatterlist sg = {0};
> +       unsigned char *blocks = NULL;
> +       size_t data_bytes;
> +       int err;
> +
> +       /*
> +        * Only allow ACMDs on the whole block device, not on partitions.  This
> +        * prevents overspray between sibling partitions.
> +        */
> +       if (bdev != bdev->bd_contains)
> +               return -EPERM;

This should at least also check CAP_SYS_ADMIN capability.

> +
> +       md = mmc_blk_get(bdev->bd_disk);
> +       if (!md)
> +               return -EINVAL;
> +
> +       card = md->queue.card;
> +       if (IS_ERR(card))
> +               return PTR_ERR(card);
> +
> +       host = card->host;
> +       mmc_claim_host(host);
> +
> +       err = mmc_app_cmd(host, card);
> +       if (err)
> +               goto acmd_done;
> +
> +       mrq.cmd = &cmd;
> +       mrq.data = &data;
> +
> +       if (copy_from_user(&sdic, sdic_ptr, sizeof(sdic))) {
> +               err = -EFAULT;
> +               goto acmd_done;
> +       }

You should first copy and verify ioctl's data and only then claim host
and send commands. Preferably the check-and-copy should be separate
function.

> +
> +       cmd.opcode = sdic.opcode;
> +       cmd.arg = sdic.arg;
> +       cmd.flags = sdic.flags;
> +
> +       data.sg = &sg;
> +       data.sg_len = 1;
> +       data.blksz = sdic.blksz;
> +       data.blocks = sdic.blocks;
> +
> +       data_bytes = data.blksz * data.blocks;
> +       blocks = kzalloc(data_bytes, GFP_KERNEL);
> +       if (!blocks) {
> +               err = -ENOMEM;
> +               goto acmd_done;
> +       }
> +       sg_init_one(data.sg, blocks, data_bytes);
> +
> +
> +       if (copy_from_user(blocks, sdic_ptr->data_ptr, data_bytes)) {
> +               err = -EFAULT;
> +               goto acmd_done;
> +       }
> +       if (sdic.write_flag)
> +               data.flags = MMC_DATA_WRITE;
> +       else
> +               data.flags = MMC_DATA_READ;
> +
> +       /* data.flags must already be set before doing this. */
> +       mmc_set_data_timeout(&data, card);
> +       /* Allow overriding the timeout_ns for empirical tuning. */
> +       if (sdic.force_timeout_ns)
> +               data.timeout_ns = sdic.force_timeout_ns;
> +
> +       mmc_wait_for_req(host, &mrq);
> +
> +       if (cmd.error) {
> +               dev_err(mmc_dev(host), "%s: cmd error %d\n",
> +                                               __func__, cmd.error);
> +               err = cmd.error;
> +               goto acmd_done;
> +       }
> +       if (data.error) {
> +               dev_err(mmc_dev(host), "%s: data error %d\n",
> +                                               __func__, data.error);
> +               err = data.error;
> +               goto acmd_done;
> +       }
> +
> +       /*
> +        * According to the SD specs, some commands require a delay after
> +        * issuing the command.
> +        */
> +       if (sdic.postsleep_us)
> +               udelay(sdic.postsleep_us);
> +
> +       if (copy_to_user(&(sdic_ptr->response), cmd.resp, sizeof(cmd.resp))) {
> +               err = -EFAULT;
> +               goto acmd_done;
> +       }
> +
> +       if (!sdic.write_flag) {
> +               if (copy_to_user(sdic_ptr->data_ptr, blocks, data_bytes)) {
> +                       err = -EFAULT;
> +                       goto acmd_done;
> +               }
> +       }
> +
> +acmd_done:
> +       kfree(blocks);
> +       mmc_release_host(host);
> +       mmc_blk_put(md);
> +       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 == SD_IOC_ACMD)
> +               ret = mmc_blk_ioctl_acmd(bdev, (struct sd_ioc_cmd __user *)arg);
> +       mutex_unlock(&block_mutex);
> +       return ret;
> +}
> +
> +#ifdef CONFIG_COMPAT
> +struct sd_ioc_cmd32 {
> +       u32 write_flag;
> +       u32 opcode;
> +       u32 arg;
> +       u32 response[4];
> +       u32 flags;
> +       u32 blksz;
> +       u32 blocks;
> +       u32 postsleep_us;
> +       u32 force_timeout_ns;
> +       compat_uptr_t data_ptr;
> +};
> +#define SD_IOC_ACMD32 _IOWR(MMC_BLOCK_MAJOR, 0, struct sd_ioc_cmd32)
[...]

Since your implementing a new ioctl you can make the structure the
same for 64 and 32-bit archs and avoid all this compat crap.

Best Regards,
Michał Mirosław