[PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs

[PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs

[John Calixto]

Part 3 of the SD Specification (SD Card Association; www.sdcard.org) describes
how to use the security function of an SD card using application specific
commands in conjunction with CPRM algorithms and keys licensed from the 4C
Entity (www.4centity.com).  This allows userspace applications to access this
security feature.

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>
---
(I'm resending because I forgot to cc the maintainer - sorry!)

 drivers/mmc/card/block.c  |  149 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/core/sd_ops.c |    3 +-
 include/linux/mmc/core.h  |    1 +
 include/linux/mmc/sd.h    |   18 ++++++
 4 files changed, 170 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index bfc8a8a..62f742d 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -39,6 +39,7 @@

 #include <asm/system.h>
 #include <asm/uaccess.h>
+#include <asm/delay.h>

 #include "queue.h"

@@ -158,11 +159,159 @@ mmc_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo)
        return 0;
 }

+static int mmc_blk_ioctl(struct block_device *bdev, fmode_t mode, unsigned int ioc_cmd, unsigned long ioc_arg)
+{
+       struct sd_ioc_cmd sdic = {0};
+       struct mmc_blk_data *md = NULL;
+       struct mmc_host *host = NULL;
+       struct mmc_card *card = NULL;
+       struct mmc_command cmd = {0};
+       struct mmc_data data = {0};
+       int err = 0;
+       struct mmc_request mrq = {0};
+       struct scatterlist sg = {0};
+       unsigned char *blocks = NULL;
+       size_t data_bytes = 0;
+#ifdef CONFIG_MMC_DEBUG
+       char dbgbuf[64] = {0};
+#endif
+
+       /*
+        * This is primarily used for application specific commands (ACMD), so
+        * the current ioc_cmd validation is trivial.
+        */
+       if (ioc_cmd != SD_IOC_ACMD)
+               return -EINVAL;
+
+       if (copy_from_user(&sdic, (void __user *) ioc_arg, sizeof(struct sd_ioc_cmd))) {
+               printk(KERN_ERR "%s: error reading ioctl arg from userspace\n", __func__);
+               return -EFAULT;
+       }
+
+       if (sdic.struct_version != SD_IOC_CMD_STRUCT_VERSION)
+               return -EINVAL;
+
+       /* Find the mmc structures based on the bdev. */
+       md = mmc_blk_get(bdev->bd_disk);
+       if (!md)
+               return -EINVAL;
+
+       card = md->queue.card;
+#ifdef CONFIG_MMC_DEBUG
+       printk(KERN_DEBUG "%s: card = %p\n", __func__, card);
+#endif
+       if (IS_ERR(card))
+               return PTR_ERR(card);
+
+#ifdef CONFIG_MMC_DEBUG
+       printk(KERN_DEBUG "%s: host = %p\n", __func__, card->host);
+#endif
+       host = card->host;
+       BUG_ON(!host);
+
+       mmc_claim_host(host);
+
+       err = mmc_app_cmd(host, card);
+       if (err) {
+               dev_err(mmc_dev(host), "%s: CMD%d error\n", __func__, MMC_APP_CMD);
+               goto ioctl_done;
+       }
+
+       mrq.cmd = &cmd;
+       mrq.data = &data;
+
+       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 = (unsigned char *) kzalloc(data_bytes, GFP_KERNEL);
+       if (!blocks) {
+               err = -ENOMEM;
+               goto ioctl_done;
+       }
+       sg_init_one(data.sg, blocks, data_bytes);
+
+
+       if (copy_from_user(blocks, sdic.data, data_bytes)) {
+               dev_err(mmc_dev(host), "%s: error reading userspace buffer\n", __func__);
+               err = -EFAULT;
+               goto ioctl_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;
+
+#ifdef CONFIG_MMC_DEBUG
+       hex_dump_to_buffer(blocks, data_bytes, 16, 1, dbgbuf, sizeof(dbgbuf), 0);
+       dev_dbg(mmc_dev(host), "%s: first bytes of pre data\n%s\n", __func__, dbgbuf);
+#endif
+
+       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 ioctl_done;
+       }
+       if (data.error) {
+               dev_err(mmc_dev(host), "%s: data error %d\n", __func__, data.error);
+               err = data.error;
+               goto ioctl_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(&(((struct sd_ioc_cmd *) ioc_arg)->response), cmd.resp, sizeof(u32) * 4)) {
+               dev_err(mmc_dev(host), "%s: error copying response\n", __func__);
+               err = -EFAULT;
+               goto ioctl_done;
+       }
+
+#ifdef CONFIG_MMC_DEBUG
+       hex_dump_to_buffer(blocks, data_bytes, 16, 1, dbgbuf, sizeof(dbgbuf), 0);
+       dev_dbg(mmc_dev(host), "%s: first bytes of post data\n%s\n", __func__, dbgbuf);
+#endif
+       if (!sdic.write_flag) {
+               if (copy_to_user(sdic.data, blocks, data_bytes)) {
+                       dev_err(mmc_dev(host), "%s: error copying data\n", __func__);
+                       err = -EFAULT;
+                       goto ioctl_done;
+               }
+       }
+
+ioctl_done:
+       if (blocks)
+               kfree(blocks);
+       mmc_release_host(host);
+       mmc_blk_put(md);
+       return err;
+}
+
 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,
 };

 struct mmc_blk_request {
diff --git a/drivers/mmc/core/sd_ops.c b/drivers/mmc/core/sd_ops.c
index 797cdb5..0453dcd 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(mmc_app_cmd);

 /**
  *     mmc_wait_for_app_cmd - start an application command and wait for
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 64e013f..1adda405 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/sd.h b/include/linux/mmc/sd.h
index 3fd85e0..a031cba 100644
--- a/include/linux/mmc/sd.h
+++ b/include/linux/mmc/sd.h
@@ -12,6 +12,8 @@
 #ifndef MMC_SD_H
 #define MMC_SD_H

+#include <linux/ioctl.h>
+
 /* SD commands                           type  argument     response */
   /* class 0 */
 /* This is basically the same command as for MMC with some quirks. */
@@ -84,5 +86,21 @@
 #define SD_SWITCH_ACCESS_DEF   0
 #define SD_SWITCH_ACCESS_HS    1

+struct sd_ioc_cmd {
+    unsigned int struct_version;
+#define SD_IOC_CMD_STRUCT_VERSION 0
+    int write_flag;  /* implies direction of data.  true = write, false = read */
+    unsigned int opcode;
+    unsigned int arg;
+    unsigned int flags;
+    unsigned int postsleep_us;  /* apply usecond delay *after* issuing command */
+    unsigned int force_timeout_ns;  /* force timeout to be force_timeout_ns ns */
+    unsigned int response[4];  /* CMD response */
+    unsigned int blksz;
+    unsigned int blocks;
+    unsigned char *data;  /* DAT buffer */
+};
+#define SD_IOC_ACMD _IOWR(MMC_BLOCK_MAJOR, 0, struct sd_ioc_cmd *)
+
 #endif

--
1.7.4.1

This message contains information which is confidential and privileged.  Unless you are the addressee (or authorized to receive for the addressee), you may not use, copy or disclose to anyone the message or any information contained in the message. If you have received the message in error, please advise the sender by reply e-mail and delete the message.

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs

[Ben Dooks]

On Thu, Mar 17, 2011 at 11:28:55AM -0700, John Calixto wrote:
> Part 3 of the SD Specification (SD Card Association; www.sdcard.org) describes
> how to use the security function of an SD card using application specific
> commands in conjunction with CPRM algorithms and keys licensed from the 4C
> Entity (www.4centity.com).  This allows userspace applications to access this
> security feature.
> 
> 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>
> ---
> (I'm resending because I forgot to cc the maintainer - sorry!)
> 
>  drivers/mmc/card/block.c  |  149 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/mmc/core/sd_ops.c |    3 +-
>  include/linux/mmc/core.h  |    1 +
>  include/linux/mmc/sd.h    |   18 ++++++
>  4 files changed, 170 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index bfc8a8a..62f742d 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -39,6 +39,7 @@
> 
>  #include <asm/system.h>
>  #include <asm/uaccess.h>
> +#include <asm/delay.h>
> 
>  #include "queue.h"
> 
> @@ -158,11 +159,159 @@ mmc_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo)
>         return 0;
>  }
> 
> +static int mmc_blk_ioctl(struct block_device *bdev, fmode_t mode, unsigned int ioc_cmd, unsigned long ioc_arg)
> +{
> +       struct sd_ioc_cmd sdic = {0};
> +       struct mmc_blk_data *md = NULL;
> +       struct mmc_host *host = NULL;
> +       struct mmc_card *card = NULL;
> +       struct mmc_command cmd = {0};
> +       struct mmc_data data = {0};
> +       int err = 0;
> +       struct mmc_request mrq = {0};
> +       struct scatterlist sg = {0};
> +       unsigned char *blocks = NULL;
> +       size_t data_bytes = 0;
> +#ifdef CONFIG_MMC_DEBUG
> +       char dbgbuf[64] = {0};
> +#endif
> +
> +       /*
> +        * This is primarily used for application specific commands (ACMD), so
> +        * the current ioc_cmd validation is trivial.
> +        */
> +       if (ioc_cmd != SD_IOC_ACMD)
> +               return -EINVAL;
> +
> +       if (copy_from_user(&sdic, (void __user *) ioc_arg, sizeof(struct sd_ioc_cmd))) {
> +               printk(KERN_ERR "%s: error reading ioctl arg from userspace\n", __func__);
> +               return -EFAULT;
> +       }
> +
> +       if (sdic.struct_version != SD_IOC_CMD_STRUCT_VERSION)
> +               return -EINVAL;
> +
> +       /* Find the mmc structures based on the bdev. */
> +       md = mmc_blk_get(bdev->bd_disk);
> +       if (!md)
> +               return -EINVAL;
> +
> +       card = md->queue.card;
> +#ifdef CONFIG_MMC_DEBUG
> +       printk(KERN_DEBUG "%s: card = %p\n", __func__, card);
> +#endif
> +       if (IS_ERR(card))
> +               return PTR_ERR(card);
> +
> +#ifdef CONFIG_MMC_DEBUG
> +       printk(KERN_DEBUG "%s: host = %p\n", __func__, card->host);
> +#endif
> +       host = card->host;
> +       BUG_ON(!host);
> +
> +       mmc_claim_host(host);
> +
> +       err = mmc_app_cmd(host, card);
> +       if (err) {
> +               dev_err(mmc_dev(host), "%s: CMD%d error\n", __func__, MMC_APP_CMD);
> +               goto ioctl_done;
> +       }
> +
> +       mrq.cmd = &cmd;
> +       mrq.data = &data;
> +
> +       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 = (unsigned char *) kzalloc(data_bytes, GFP_KERNEL);

you shouldn't need this cast, 'void *' will go hapilly to any other type.

> +       if (!blocks) {
> +               err = -ENOMEM;
> +               goto ioctl_done;
> +       }
> +       sg_init_one(data.sg, blocks, data_bytes);
> +
> +
> +       if (copy_from_user(blocks, sdic.data, data_bytes)) {
> +               dev_err(mmc_dev(host), "%s: error reading userspace buffer\n", __func__);
> +               err = -EFAULT;
> +               goto ioctl_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;
> +
> +#ifdef CONFIG_MMC_DEBUG
> +       hex_dump_to_buffer(blocks, data_bytes, 16, 1, dbgbuf, sizeof(dbgbuf), 0);
> +       dev_dbg(mmc_dev(host), "%s: first bytes of pre data\n%s\n", __func__, dbgbuf);
> +#endif
> +
> +       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 ioctl_done;
> +       }
> +       if (data.error) {
> +               dev_err(mmc_dev(host), "%s: data error %d\n", __func__, data.error);
> +               err = data.error;
> +               goto ioctl_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(&(((struct sd_ioc_cmd *) ioc_arg)->response), cmd.resp, sizeof(u32) * 4)) {
> +               dev_err(mmc_dev(host), "%s: error copying response\n", __func__);
> +               err = -EFAULT;
> +               goto ioctl_done;
> +       }
> +
> +#ifdef CONFIG_MMC_DEBUG
> +       hex_dump_to_buffer(blocks, data_bytes, 16, 1, dbgbuf, sizeof(dbgbuf), 0);
> +       dev_dbg(mmc_dev(host), "%s: first bytes of post data\n%s\n", __func__, dbgbuf);
> +#endif
> +       if (!sdic.write_flag) {
> +               if (copy_to_user(sdic.data, blocks, data_bytes)) {
> +                       dev_err(mmc_dev(host), "%s: error copying data\n", __func__);
> +                       err = -EFAULT;
> +                       goto ioctl_done;
> +               }
> +       }
> +
> +ioctl_done:
> +       if (blocks)
> +               kfree(blocks);
> +       mmc_release_host(host);
> +       mmc_blk_put(md);
> +       return err;
> +}
> +
>  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,
>  };
> 
>  struct mmc_blk_request {
> diff --git a/drivers/mmc/core/sd_ops.c b/drivers/mmc/core/sd_ops.c
> index 797cdb5..0453dcd 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(mmc_app_cmd);
> 
>  /**
>   *     mmc_wait_for_app_cmd - start an application command and wait for
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index 64e013f..1adda405 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/sd.h b/include/linux/mmc/sd.h
> index 3fd85e0..a031cba 100644
> --- a/include/linux/mmc/sd.h
> +++ b/include/linux/mmc/sd.h
> @@ -12,6 +12,8 @@
>  #ifndef MMC_SD_H
>  #define MMC_SD_H
> 
> +#include <linux/ioctl.h>
> +
>  /* SD commands                           type  argument     response */
>    /* class 0 */
>  /* This is basically the same command as for MMC with some quirks. */
> @@ -84,5 +86,21 @@
>  #define SD_SWITCH_ACCESS_DEF   0
>  #define SD_SWITCH_ACCESS_HS    1
> 
> +struct sd_ioc_cmd {
> +    unsigned int struct_version;
> +#define SD_IOC_CMD_STRUCT_VERSION 0
> +    int write_flag;  /* implies direction of data.  true = write, false = read */
> +    unsigned int opcode;
> +    unsigned int arg;
> +    unsigned int flags;
> +    unsigned int postsleep_us;  /* apply usecond delay *after* issuing command */
> +    unsigned int force_timeout_ns;  /* force timeout to be force_timeout_ns ns */
> +    unsigned int response[4];  /* CMD response */
> +    unsigned int blksz;
> +    unsigned int blocks;
> +    unsigned char *data;  /* DAT buffer */
> +};
> +#define SD_IOC_ACMD _IOWR(MMC_BLOCK_MAJOR, 0, struct sd_ioc_cmd *)
> +
>  #endif
> 
> --
> 1.7.4.1
> 
> This message contains information which is confidential and privileged.  Unless you are the addressee (or authorized to receive for the addressee), you may not use, copy or disclose to anyone the message or any information contained in the message. If you have received the message in error, please advise the sender by reply e-mail and delete the message.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ben Dooks, ben@fluff.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs

[Arnd Bergmann]

On Thursday 17 March 2011 19:28:55 John Calixto wrote:
> Part 3 of the SD Specification (SD Card Association; www.sdcard.org) describes
> how to use the security function of an SD card using application specific
> commands in conjunction with CPRM algorithms and keys licensed from the 4C
> Entity (www.4centity.com).  This allows userspace applications to access this
> security feature.

Having the ability to send commands from user space sounds useful,
a number of other block drivers can do this, too.

However, for the specific example you mention, I think it would be
nicer to implement it in kernel space, and have a high-level
interface.

A few more comments about the implementation below.

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

Is this safe to do while the device is operating and a file system
writes data?

Does it allow sending all commands or just the ones you require?

I've been thinking about adding something like this to allow
a passthrough mode for e.g. qemu, or perhaps for combining
it with a user space mmc host (to be written) to allow tracing
the communication between host and card from user space.

> +{
> +       struct sd_ioc_cmd sdic = {0};
> +       struct mmc_blk_data *md = NULL;
> +       struct mmc_host *host = NULL;
> +       struct mmc_card *card = NULL;
> +       struct mmc_command cmd = {0};
> +       struct mmc_data data = {0};
> +       int err = 0;
> +       struct mmc_request mrq = {0};
> +       struct scatterlist sg = {0};
> +       unsigned char *blocks = NULL;
> +       size_t data_bytes = 0;

[style] Don't initialize variables to zero if they get assigned
to something else later, otherwise you prevent the compiler
from warning you about accesses to uninitialized variables.

> +#ifdef CONFIG_MMC_DEBUG
> +       char dbgbuf[64] = {0};
> +#endif

> +
> +       if (copy_from_user(&sdic, (void __user *) ioc_arg, sizeof(struct sd_ioc_cmd))) {
> +               printk(KERN_ERR "%s: error reading ioctl arg from userspace\n", __func__);
> +               return -EFAULT;
> +       }

Don't printk information about user input, only about unexpected
data you get from the card. If you print an error, use dev_err().

> +       if (sdic.struct_version != SD_IOC_CMD_STRUCT_VERSION)
> +               return -EINVAL;

Don't use version information for ioctl data structures. We cannot
change them anyways, so the version number is redundant.
If we ever need to extend the structure, it has to be done in
a compatible way, or by introducing a new ioctl command with
a different structure, but leaving the old one in place.

> +       /* Find the mmc structures based on the bdev. */
> +       md = mmc_blk_get(bdev->bd_disk);
> +       if (!md)
> +               return -EINVAL;
> +
> +       card = md->queue.card;
> +#ifdef CONFIG_MMC_DEBUG
> +       printk(KERN_DEBUG "%s: card = %p\n", __func__, card);
> +#endif

dev_dbg()

> +       if (IS_ERR(card))
> +               return PTR_ERR(card);
> +
> +#ifdef CONFIG_MMC_DEBUG
> +       printk(KERN_DEBUG "%s: host = %p\n", __func__, card->host);
> +#endif

dev_dbg()

> +       host = card->host;
> +       BUG_ON(!host);

try to be very careful about BUG_ON(), it's always better to return
an error if you can avoid crashing the current process.

> +       mmc_claim_host(host);
> +
> +       err = mmc_app_cmd(host, card);
> +       if (err) {
> +               dev_err(mmc_dev(host), "%s: CMD%d error\n", __func__, MMC_APP_CMD);
> +               goto ioctl_done;
> +       }

Is this an unexpected error case? If it can be triggered by sending
specific commands, it would be better to just return the error
number ot user space without printing anything.

> +       data_bytes = data.blksz * data.blocks;
> +       blocks = (unsigned char *) kzalloc(data_bytes, GFP_KERNEL);

No need for the typecast, kzalloc returns a void pointer.

> +       if (copy_from_user(blocks, sdic.data, data_bytes)) {
> +               dev_err(mmc_dev(host), "%s: error reading userspace buffer\n", __func__);

no printing here again

> +#ifdef CONFIG_MMC_DEBUG
> +       hex_dump_to_buffer(blocks, data_bytes, 16, 1, dbgbuf, sizeof(dbgbuf), 0);
> +       dev_dbg(mmc_dev(host), "%s: first bytes of pre data\n%s\n", __func__, dbgbuf);
> +#endif

Is this really still needed? I would assume that if your code works
fine, you can just remove the hex dump.

>  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,
>  };

This also needs a compat_ioctl pointer, new drivers should always
work in both 32 and 64 bit mode.
 
> diff --git a/drivers/mmc/core/sd_ops.c b/drivers/mmc/core/sd_ops.c
> index 797cdb5..0453dcd 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(mmc_app_cmd);
 
Why not EXPORT_SYMBOL_GPL?

> @@ -84,5 +86,21 @@
>  #define SD_SWITCH_ACCESS_DEF   0
>  #define SD_SWITCH_ACCESS_HS    1
> 
> +struct sd_ioc_cmd {
> +    unsigned int struct_version;
> +#define SD_IOC_CMD_STRUCT_VERSION 0
> +    int write_flag;  /* implies direction of data.  true = write, false = read */
> +    unsigned int opcode;
> +    unsigned int arg;
> +    unsigned int flags;
> +    unsigned int postsleep_us;  /* apply usecond delay *after* issuing command */
> +    unsigned int force_timeout_ns;  /* force timeout to be force_timeout_ns ns */
> +    unsigned int response[4];  /* CMD response */
> +    unsigned int blksz;
> +    unsigned int blocks;
> +    unsigned char *data;  /* DAT buffer */
> +};
> +#define SD_IOC_ACMD _IOWR(MMC_BLOCK_MAJOR, 0, struct sd_ioc_cmd *)
> +
>  #endif
> 

As I mentioned, any ioctl command that gets introduced needs to be
designed very carefully to make sure we don't need to change it in the
future. The only two things I can see here are:

* The struct_version should be removed
* The data pointer is not compatible between 32 and 64 bits.
  One solution for this would be to make it a __u64 argument
  and require the user to cast the pointer to a 64 bit type.

	Arnd

RE: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs

[John Calixto]

Hello Arnd,

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: Thursday, March 17, 2011 2:56 PM
> To: John Calixto
> Cc: linux-mmc@vger.kernel.org; cjb@laptop.org
> Subject: Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs
>
> On Thursday 17 March 2011 19:28:55 John Calixto wrote:
> > Part 3 of the SD Specification (SD Card Association; www.sdcard.org) describes
> > how to use the security function of an SD card using application specific
> > commands in conjunction with CPRM algorithms and keys licensed from the 4C
> > Entity (www.4centity.com).  This allows userspace applications to access this
> > security feature.
>
> Having the ability to send commands from user space sounds useful,
> a number of other block drivers can do this, too.
>
> However, for the specific example you mention, I think it would be
> nicer to implement it in kernel space, and have a high-level
> interface.

I started down that route, but part of the problem with putting any more
than a simple passthrough in kernel space is that the CPRM algorithms
live in the next highest logic layer, and 4C licensees are not allowed
to reveal those algorithms.  If you have access to the SD Specification,
you will see that it documents all of the individual security commands.
However, the sequence of commands is documented in the 4C CPRM
Specification.

Installing this passthrough also has the added benefit of allowing
other, non-security-related, application specific commands to be sent.

>
> A few more comments about the implementation below.
>
> > Tested on TI PCIxx12 (SDHCI), Sigma Designs SMP8652 SoC, TI OMAP3621 SoC, TI
> > OMAP3630 SoC, Samsung S5PC110 SoC, Qualcomm MSM7200A SoC.
>
> Is this safe to do while the device is operating and a file system
> writes data?

If you mean:

    Is it safe for one process to write data to the User Area (i.e. the
    non-secure part; the normal use case for SD as unencrypted storage),
    while another process issues secure commands?

then, no, it is not.  Should I try and protect against this in the
kernel driver?

If you mean:

    Can a program use both the User Area and the Protected Area?

then, yes, it can.  In broad terms, the typical program would:

    - mount /dev/mmcblk0p1 /mnt/sdcard
    - find encrypted file /mnt/sdcard/encrypted_blob
    - issue secure commands via ioctl() to /dev/mmcblk0p1 to find
      decryption keys; no need to umount
    - decrypt /mnt/sdcard/encrypted_blob using keys retrieved above

>
> Does it allow sending all commands or just the ones you require?
>

I did not choose to filter out commands.  I expected that other folks
might want to send other ACMDs.

<snip>

I'll apply the rest of your feedback and Ben Dooks' tip about the
unnecessary cast in the next revision of the patch.

Thanks!

John

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs

[Michał Mirosław]

2011/3/18 John Calixto <john.calixto@modsystems.com>:
>> -----Original Message-----
>> From: Arnd Bergmann [mailto:arnd@arndb.de]
>> Sent: Thursday, March 17, 2011 2:56 PM
>> To: John Calixto
>> Cc: linux-mmc@vger.kernel.org; cjb@laptop.org
>> Subject: Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs
>>
>> On Thursday 17 March 2011 19:28:55 John Calixto wrote:
>> > Part 3 of the SD Specification (SD Card Association; www.sdcard.org) describes
>> > how to use the security function of an SD card using application specific
>> > commands in conjunction with CPRM algorithms and keys licensed from the 4C
>> > Entity (www.4centity.com).  This allows userspace applications to access this
>> > security feature.
>>
>> Having the ability to send commands from user space sounds useful,
>> a number of other block drivers can do this, too.
>>
>> However, for the specific example you mention, I think it would be
>> nicer to implement it in kernel space, and have a high-level
>> interface.
>
> I started down that route, but part of the problem with putting any more
> than a simple passthrough in kernel space is that the CPRM algorithms
> live in the next highest logic layer, and 4C licensees are not allowed
> to reveal those algorithms.  If you have access to the SD Specification,
> you will see that it documents all of the individual security commands.
> However, the sequence of commands is documented in the 4C CPRM
> Specification.
>
> Installing this passthrough also has the added benefit of allowing
> other, non-security-related, application specific commands to be sent.

If that's going to be used by possibly unprivileged userspace process,
then this passthrough should filter and validate all commands it
passes to hardware. If there is a possibility of some command sequence
to generate undefined or otherwise unwanted results, then you need
state tracker that will disallow that sequence to be generated by
unprivileged process.

Best Regards,
Michał Mirosław

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs

[Arnd Bergmann]

On Friday 18 March 2011 18:32:41 John Calixto wrote:
> I started down that route, but part of the problem with putting any more
> than a simple passthrough in kernel space is that the CPRM algorithms
> live in the next highest logic layer, and 4C licensees are not allowed
> to reveal those algorithms.  If you have access to the SD Specification,
> you will see that it documents all of the individual security commands.
> However, the sequence of commands is documented in the 4C CPRM
> Specification.

Implementing this without revealing the algorithm is hardly possible:
As soon as there is an implementation in binary form, someone can
reverse-engineer and document it, and then we can write a proper
implementation in the kernel. The question is whether we can skip
this detour and get the correct implementation right-away ;-)

Since you have already signed an NDA, you can obviously not
send the implementation for this, but someone else could
fill in the blanks.

> Installing this passthrough also has the added benefit of allowing
> other, non-security-related, application specific commands to be sent.

Yes, I understand that part, and I think that's good, although
it would still be better to do the CPRM stuff in the kernel,
as I said.

> > A few more comments about the implementation below.
> >
> > > Tested on TI PCIxx12 (SDHCI), Sigma Designs SMP8652 SoC, TI OMAP3621 SoC, TI
> > > OMAP3630 SoC, Samsung S5PC110 SoC, Qualcomm MSM7200A SoC.
> >
> > Is this safe to do while the device is operating and a file system
> > writes data?
> 
> If you mean:
> 
>     Is it safe for one process to write data to the User Area (i.e. the
>     non-secure part; the normal use case for SD as unencrypted storage),
>     while another process issues secure commands?
> 
> then, no, it is not.  Should I try and protect against this in the
> kernel driver?

I guess that would be better. You cannot really stop the kernel
from issuing block commands on a mounted file system from user
space, which appears to be required here.

> If you mean:
> 
>     Can a program use both the User Area and the Protected Area?
> 
> then, yes, it can.  In broad terms, the typical program would:
> 
>     - mount /dev/mmcblk0p1 /mnt/sdcard
>     - find encrypted file /mnt/sdcard/encrypted_blob
>     - issue secure commands via ioctl() to /dev/mmcblk0p1 to find
>       decryption keys; no need to umount
>     - decrypt /mnt/sdcard/encrypted_blob using keys retrieved above

What if /dev/mmcblk0 contains your root file system and/or
swap space, and the program that uses ioctl here happens
to be paged out?

You may get into a situation where the card is owned by the
program that needs to complete talking to it before the kernel
can do more block commands, but the program cannot continue until
the block driver reads back the program .text section from the
card.

> > Does it allow sending all commands or just the ones you require?
> >
> 
> I did not choose to filter out commands.  I expected that other folks
> might want to send other ACMDs.

What about regular commands, read/write/erase or the more
sophisticated ones? We might not want to do those on a mounted
card, but it would be useful for the case where you pass through
a real SD card to a virtual machine using qemu.

	Arnd

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs

[Arnd Bergmann]

On Friday 18 March 2011 18:56:53 Michał Mirosław wrote:
> If that's going to be used by possibly unprivileged userspace process,
> then this passthrough should filter and validate all commands it
> passes to hardware. If there is a possibility of some command sequence
> to generate undefined or otherwise unwanted results, then you need
> state tracker that will disallow that sequence to be generated by
> unprivileged process.

We have precedence for direct host commands in a few other
block drivers. In general, any user who can open the block
device can issue all commands unless they can directly destroy
the hardware. On normal systems, the only user that has write
access to block devices is root.

	Arnd

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMD

[John Calixto]

On Fri, 18 Mar 2011, Arnd Bergmann wrote:
> On Friday 18 March 2011 18:32:41 John Calixto wrote:
> > I started down that route, but part of the problem with putting any more
> > than a simple passthrough in kernel space is that the CPRM algorithms
> > live in the next highest logic layer, and 4C licensees are not allowed
> > to reveal those algorithms.  If you have access to the SD Specification,
> > you will see that it documents all of the individual security commands.
> > However, the sequence of commands is documented in the 4C CPRM
> > Specification.
> 
> Implementing this without revealing the algorithm is hardly possible:
> As soon as there is an implementation in binary form, someone can
> reverse-engineer and document it, and then we can write a proper
> implementation in the kernel. The question is whether we can skip
> this detour and get the correct implementation right-away ;-)
> 

Certainly, physical access to any device makes reverse engineering of
any of these 'security' techniques possible to anyone with time and
patience.  That said, I am not in a position to help with this - Sorry!

> Since you have already signed an NDA, you can obviously not
> send the implementation for this, but someone else could
> fill in the blanks.
> 
> > Installing this passthrough also has the added benefit of allowing
> > other, non-security-related, application specific commands to be sent.
> 
> Yes, I understand that part, and I think that's good, although
> it would still be better to do the CPRM stuff in the kernel,
> as I said.
> 

This is a matter to take up with the 4C.  Looking at the rest of the
MMC/SD code in the kernel, it appears that somebody with access to the
SD specs got permission to implement what we have now.  Perhaps they
could help with filling in the blanks?  IANAL, so the patch I am
submitting does not include proprietary elements.  However, it does
allow anyone with access to the specs a way to use what they know.

> > > A few more comments about the implementation below.
> > >
> > > > Tested on TI PCIxx12 (SDHCI), Sigma Designs SMP8652 SoC, TI OMAP3621 SoC, TI
> > > > OMAP3630 SoC, Samsung S5PC110 SoC, Qualcomm MSM7200A SoC.
> > >
> > > Is this safe to do while the device is operating and a file system
> > > writes data?
> > 
> > If you mean:
> > 
> >     Is it safe for one process to write data to the User Area (i.e. the
> >     non-secure part; the normal use case for SD as unencrypted storage),
> >     while another process issues secure commands?
> > 
> > then, no, it is not.  Should I try and protect against this in the
> > kernel driver?
> 
> I guess that would be better. You cannot really stop the kernel
> from issuing block commands on a mounted file system from user
> space, which appears to be required here.
> 

Actually, I got ahead of myself in my initial response.  I believe I
already protected against concurrent access with mmc_claim_host().

> > If you mean:
> > 
> >     Can a program use both the User Area and the Protected Area?
> > 
> > then, yes, it can.  In broad terms, the typical program would:
> > 
> >     - mount /dev/mmcblk0p1 /mnt/sdcard
> >     - find encrypted file /mnt/sdcard/encrypted_blob
> >     - issue secure commands via ioctl() to /dev/mmcblk0p1 to find
> >       decryption keys; no need to umount
> >     - decrypt /mnt/sdcard/encrypted_blob using keys retrieved above
> 
> What if /dev/mmcblk0 contains your root file system and/or
> swap space, and the program that uses ioctl here happens
> to be paged out?
> 
> You may get into a situation where the card is owned by the
> program that needs to complete talking to it before the kernel
> can do more block commands, but the program cannot continue until
> the block driver reads back the program .text section from the
> card.
> 

Having used mmc_claim_host() (see my previous comment above), the ioctl
should complete before the kernel gets a chance to page the program out
shouldn't it?

> > > Does it allow sending all commands or just the ones you require?
> > >
> > 
> > I did not choose to filter out commands.  I expected that other folks
> > might want to send other ACMDs.
> 
> What about regular commands, read/write/erase or the more
> sophisticated ones? We might not want to do those on a mounted
> card, but it would be useful for the case where you pass through
> a real SD card to a virtual machine using qemu.
> 
> 	Arnd

The patch is only for ACMDs for now.  When calling the ioctl with
cmd=SD_IOC_ACMD, it implies you expect CMD55 to be sent *before* sending
your command.  There are specs that describe the values and behaviours
of ACMDs.  You'll find that the SD card's state machine is pretty
resilient, and that apps that are messing with tossing garbage ACMDs in
should expect garbage back out.

I expect for more general purposes, like your qemu passthrough case, you
would want to add a handler for cmd=SD_IOC_CMD?

RE: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs

[John Calixto]

Hello Arnd,

I need some clarification on the last bit of your initial feedback below:

On Thu, 17 Mar 2011, Arnd Bergmann wrote:
> On Thursday 17 March 2011 19:28:55 John Calixto wrote:

<snip>

> > diff --git a/drivers/mmc/core/sd_ops.c b/drivers/mmc/core/sd_ops.c
> > index 797cdb5..0453dcd 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(mmc_app_cmd);
>
> Why not EXPORT_SYMBOL_GPL?
>

I was just using the convention already used in sd_ops.c.  I can send a
pre-patch that sets all of the symbol exports in that file to be
EXPORT_SYMBOL_GPL, but without confirmation from you and others on this
list, that seems like overstepping my "jurisdiction".  Is that preferable?

> > @@ -84,5 +86,21 @@
> >  #define SD_SWITCH_ACCESS_DEF   0
> >  #define SD_SWITCH_ACCESS_HS    1
> >
> > +struct sd_ioc_cmd {
> > +    unsigned int struct_version;
> > +#define SD_IOC_CMD_STRUCT_VERSION 0
> > +    int write_flag;  /* implies direction of data.  true = write, false = read */
> > +    unsigned int opcode;
> > +    unsigned int arg;
> > +    unsigned int flags;
> > +    unsigned int postsleep_us;  /* apply usecond delay *after* issuing command */
> > +    unsigned int force_timeout_ns;  /* force timeout to be force_timeout_ns ns */
> > +    unsigned int response[4];  /* CMD response */
> > +    unsigned int blksz;
> > +    unsigned int blocks;
> > +    unsigned char *data;  /* DAT buffer */
> > +};
> > +#define SD_IOC_ACMD _IOWR(MMC_BLOCK_MAJOR, 0, struct sd_ioc_cmd *)
> > +
> >  #endif
> >
>
> As I mentioned, any ioctl command that gets introduced needs to be
> designed very carefully to make sure we don't need to change it in the
> future. The only two things I can see here are:
>
> * The struct_version should be removed
> * The data pointer is not compatible between 32 and 64 bits.
>   One solution for this would be to make it a __u64 argument
>   and require the user to cast the pointer to a 64 bit type.
>
>       Arnd

I don't understand your comment about the data pointer not being
compatible between 32 and 64 bits.  Wouldn't the compiler take care of
pointer size?

Thanks,

John

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs

[Arnd Bergmann]

On Saturday 19 March 2011, John Calixto wrote:

> > > -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(mmc_app_cmd);
> >
> > Why not EXPORT_SYMBOL_GPL?
> >
> 
> I was just using the convention already used in sd_ops.c.  I can send a
> pre-patch that sets all of the symbol exports in that file to be
> EXPORT_SYMBOL_GPL, but without confirmation from you and others on this
> list, that seems like overstepping my "jurisdiction".  Is that preferable?

No, you shouldn't change existing symbols, we normally don't do that.
Some subsystems in the kernel generally allow regular EXPORT_SYMBOL
for everything, I don't know if that's the case here.

Chris, do you prefer to leave the new export as EXPORT_SYMBOL
along wiht the others or to use EXPORT_SYMBOL_GPL?

> > As I mentioned, any ioctl command that gets introduced needs to be
> > designed very carefully to make sure we don't need to change it in the
> > future. The only two things I can see here are:
> >
> > * The struct_version should be removed
> > * The data pointer is not compatible between 32 and 64 bits.
> >   One solution for this would be to make it a __u64 argument
> >   and require the user to cast the pointer to a 64 bit type.
> 
> I don't understand your comment about the data pointer not being
> compatible between 32 and 64 bits.  Wouldn't the compiler take care of
> pointer size?

The problem is when you have a 64 bit kernel that can run both
32 and 64 bit user applications. The compiler for the 32 bit user
process will create a smaller data structure than what a 64 bit
kernel expects. The two options here are to force the compiler
to use a compatible layout, or to use the compat_ioctl() function
to convert from one format to the other.

	Arnd

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMD

[Arnd Bergmann]

On Friday 18 March 2011, John Calixto wrote:
> On Fri, 18 Mar 2011, Arnd Bergmann wrote:
> > On Friday 18 March 2011 18:32:41 John Calixto wrote:
> > > I started down that route, but part of the problem with putting any more
> > > than a simple passthrough in kernel space is that the CPRM algorithms
> > > live in the next highest logic layer, and 4C licensees are not allowed
> > > to reveal those algorithms.  If you have access to the SD Specification,
> > > you will see that it documents all of the individual security commands.
> > > However, the sequence of commands is documented in the 4C CPRM
> > > Specification.
> > 
> > Implementing this without revealing the algorithm is hardly possible:
> > As soon as there is an implementation in binary form, someone can
> > reverse-engineer and document it, and then we can write a proper
> > implementation in the kernel. The question is whether we can skip
> > this detour and get the correct implementation right-away ;-)
> > 
> 
> Certainly, physical access to any device makes reverse engineering of
> any of these 'security' techniques possible to anyone with time and
> patience.  That said, I am not in a position to help with this - Sorry!

I understand your problem.

> > Since you have already signed an NDA, you can obviously not
> > send the implementation for this, but someone else could
> > fill in the blanks.
> > 
> > > Installing this passthrough also has the added benefit of allowing
> > > other, non-security-related, application specific commands to be sent.
> > 
> > Yes, I understand that part, and I think that's good, although
> > it would still be better to do the CPRM stuff in the kernel,
> > as I said.
> > 
> 
> This is a matter to take up with the 4C.  Looking at the rest of the
> MMC/SD code in the kernel, it appears that somebody with access to the
> SD specs got permission to implement what we have now.  Perhaps they
> could help with filling in the blanks?  IANAL, so the patch I am
> submitting does not include proprietary elements.  However, it does
> allow anyone with access to the specs a way to use what they know.

Many of the SD specifications have redacted versions that allow
us to write OS drivers. For instance there is
http://www.sdcard.org/developers//tech/sdcard/pls/simplified_specs/Part_A1_ASSD_Extension_Simplified_Specification_Ver2.00_Final_100518.pdf
that describes some of the "advanced security SD" card 

> > I guess that would be better. You cannot really stop the kernel
> > from issuing block commands on a mounted file system from user
> > space, which appears to be required here.
> > 
> 
> Actually, I got ahead of myself in my initial response.  I believe I
> already protected against concurrent access with mmc_claim_host().

Ok, so is the protocol designed in a way that you don't need to
mmc_claim_host() across multiple ACMDs to do CPRM?

> > You may get into a situation where the card is owned by the
> > program that needs to complete talking to it before the kernel
> > can do more block commands, but the program cannot continue until
> > the block driver reads back the program .text section from the
> > card.
> > 
> 
> Having used mmc_claim_host() (see my previous comment above), the ioctl
> should complete before the kernel gets a chance to page the program out
> shouldn't it?

Right. But is that enough? The qeustion is whether the card may be
in a state where it's expecting another ACMD after the first ioctl
and cannot deal with regular block commands.

> The patch is only for ACMDs for now.  When calling the ioctl with
> cmd=SD_IOC_ACMD, it implies you expect CMD55 to be sent *before* sending
> your command.  There are specs that describe the values and behaviours
> of ACMDs.  You'll find that the SD card's state machine is pretty
> resilient, and that apps that are messing with tossing garbage ACMDs in
> should expect garbage back out.
> 
> I expect for more general purposes, like your qemu passthrough case, you
> would want to add a handler for cmd=SD_IOC_CMD?

Yes, but I suspect that would mean blocking all other activity on the
same device, possibly with an ioctl for claiming the host for an
extended period of time.

	Arnd

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs

[Chris Ball]

Hi,

On Sat, Mar 19 2011, Arnd Bergmann wrote:
> Chris, do you prefer to leave the new export as EXPORT_SYMBOL
> along wiht the others or to use EXPORT_SYMBOL_GPL?

Let's just use EXPORT_SYMBOL_GPL() for all new exports, and leave the
older ones as they are.

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs

[Michał Mirosław]

W dniu 18 marca 2011 20:26 użytkownik Arnd Bergmann <arnd@arndb.de> napisał:
> On Friday 18 March 2011 18:56:53 Michał Mirosław wrote:
>> If that's going to be used by possibly unprivileged userspace process,
>> then this passthrough should filter and validate all commands it
>> passes to hardware. If there is a possibility of some command sequence
>> to generate undefined or otherwise unwanted results, then you need
>> state tracker that will disallow that sequence to be generated by
>> unprivileged process.
> We have precedence for direct host commands in a few other
> block drivers. In general, any user who can open the block
> device can issue all commands unless they can directly destroy
> the hardware. On normal systems, the only user that has write
> access to block devices is root.

In this case, a process having access to one partition can disrupt
other partitions on the same card even if it has no access to them in
any other way.

It is not that unusual on "normal systems" to give write access to
some partition or device to unprivileged users. Database volumes are
one example.

Best Regards,
Michał Mirosław

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs

[Arnd Bergmann]

On Saturday 19 March 2011, Michał Mirosław wrote:
> W dniu 18 marca 2011 20:26 użytkownik Arnd Bergmann <arnd@arndb.de> napisał:
> > On Friday 18 March 2011 18:56:53 Michał Mirosław wrote:
> >> If that's going to be used by possibly unprivileged userspace process,
> >> then this passthrough should filter and validate all commands it
> >> passes to hardware. If there is a possibility of some command sequence
> >> to generate undefined or otherwise unwanted results, then you need
> >> state tracker that will disallow that sequence to be generated by
> >> unprivileged process.
> > We have precedence for direct host commands in a few other
> > block drivers. In general, any user who can open the block
> > device can issue all commands unless they can directly destroy
> > the hardware. On normal systems, the only user that has write
> > access to block devices is root.
> 
> In this case, a process having access to one partition can disrupt
> other partitions on the same card even if it has no access to them in
> any other way.
> 
> It is not that unusual on "normal systems" to give write access to
> some partition or device to unprivileged users. Database volumes are
> one example.

We can probably restrict it to the actual block device, and disallow
the ioctl on partitions to avoid that problem.

	Arnd

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMD

[John Calixto]

On Sat, 19 Mar 2011, Arnd Bergmann wrote:
> Many of the SD specifications have redacted versions that allow
> us to write OS drivers. For instance there is
> http://www.sdcard.org/developers//tech/sdcard/pls/simplified_specs/Part_A1_ASSD_Extension_Simplified_Specification_Ver2.00_Final_100518.pdf
> that describes some of the "advanced security SD" card 

Thanks for the reference.  I was not aware of those docs.  Note that
anything related to CPRM is redacted.  I suspect it's because it's a
separate organization that actually publishes and licenses the CPRM
specs - the 4C Entity, LLC.  I agree with you that it would be ideal for
us if we could include much of the CPRM code directly into the kernel,
but I don't see how that's possible at this time.

> Ok, so is the protocol designed in a way that you don't need to
> mmc_claim_host() across multiple ACMDs to do CPRM?
> 
> > > You may get into a situation where the card is owned by the
> > > program that needs to complete talking to it before the kernel
> > > can do more block commands, but the program cannot continue until
> > > the block driver reads back the program .text section from the
> > > card.
> > > 
> > 
> > Having used mmc_claim_host() (see my previous comment above), the ioctl
> > should complete before the kernel gets a chance to page the program out
> > shouldn't it?
> 
> Right. But is that enough? The qeustion is whether the card may be
> in a state where it's expecting another ACMD after the first ioctl
> and cannot deal with regular block commands.
> 

I see your point.  The card certainly will not allow access to the
secure area if you interleave secure commands with regular, non-secure
commands - Failed secure ops would be the immediately observable
behaviour that I would expect.  However, I am not sure about the
potential negative effects on the regular block operations.  I will go
back and specifically test for these scenarios.  Though I must say that
in the thousands of hours of manual and automated testing, and at least
hundreds of hours of use by end-users, this patch has held its own.

Again, I will go back and specifically focus on breaking regular block
operations with interleaved injections of ACMDs using this ioctl.

> > The patch is only for ACMDs for now.  When calling the ioctl with
> > cmd=SD_IOC_ACMD, it implies you expect CMD55 to be sent *before* sending
> > your command.  There are specs that describe the values and behaviours
> > of ACMDs.  You'll find that the SD card's state machine is pretty
> > resilient, and that apps that are messing with tossing garbage ACMDs in
> > should expect garbage back out.
> > 
> > I expect for more general purposes, like your qemu passthrough case, you
> > would want to add a handler for cmd=SD_IOC_CMD?
> 
> Yes, but I suspect that would mean blocking all other activity on the
> same device, possibly with an ioctl for claiming the host for an
> extended period of time.
> 

Sure.  And if the testing I described above shows that regular block
operations are indeed negatively affected, then I'll have to implement
that ioctl for claiming the host myself and you'll have a head start! :)

John

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMD

[Michał Mirosław]

2011/3/20 John Calixto <john.calixto@modsystems.com>:
> On Sat, 19 Mar 2011, Arnd Bergmann wrote:
[...]
>> > I expect for more general purposes, like your qemu passthrough case, you
>> > would want to add a handler for cmd=SD_IOC_CMD?
>> Yes, but I suspect that would mean blocking all other activity on the
>> same device, possibly with an ioctl for claiming the host for an
>> extended period of time.
> Sure.  And if the testing I described above shows that regular block
> operations are indeed negatively affected, then I'll have to implement
> that ioctl for claiming the host myself and you'll have a head start! :)

This idea is scary. ;-) Just think what would happen if userspace made
this sequence:
1. fd = open(dev)
2. ioctl(fd, claim host)
3. read(fd, ...)

(if 3. is not convincing, replace it with any access to mounted
filesystem from the same card.)

Best Regards,
Michał Mirosław

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMD

[Arnd Bergmann]

On Sunday 20 March 2011, Michał Mirosław wrote:
> 2011/3/20 John Calixto <john.calixto@modsystems.com>:
> > On Sat, 19 Mar 2011, Arnd Bergmann wrote:
> [...]
> >> > I expect for more general purposes, like your qemu passthrough case, you
> >> > would want to add a handler for cmd=SD_IOC_CMD?
> >> Yes, but I suspect that would mean blocking all other activity on the
> >> same device, possibly with an ioctl for claiming the host for an
> >> extended period of time.
> > Sure.  And if the testing I described above shows that regular block
> > operations are indeed negatively affected, then I'll have to implement
> > that ioctl for claiming the host myself and you'll have a head start! :)
> 
> This idea is scary. ;-) Just think what would happen if userspace made
> this sequence:
> 1. fd = open(dev)
> 2. ioctl(fd, claim host)
> 3. read(fd, ...)
> 
> (if 3. is not convincing, replace it with any access to mounted
> filesystem from the same card.)

I agree. If we allow such a command on a block device, that
should probably be mutually exclusive with normal read/write
access or mounting the block device, and it needs to be a
priviledged operation.

	Arnd

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMD

[Andrei Warkentin]

2011/3/21 Arnd Bergmann <arnd@arndb.de>:
> On Sunday 20 March 2011, Michał Mirosław wrote:
>> 2011/3/20 John Calixto <john.calixto@modsystems.com>:
>> > On Sat, 19 Mar 2011, Arnd Bergmann wrote:
>> [...]
>> >> > I expect for more general purposes, like your qemu passthrough case, you
>> >> > would want to add a handler for cmd=SD_IOC_CMD?
>> >> Yes, but I suspect that would mean blocking all other activity on the
>> >> same device, possibly with an ioctl for claiming the host for an
>> >> extended period of time.
>> > Sure.  And if the testing I described above shows that regular block
>> > operations are indeed negatively affected, then I'll have to implement
>> > that ioctl for claiming the host myself and you'll have a head start! :)
>>
>> This idea is scary. ;-) Just think what would happen if userspace made
>> this sequence:
>> 1. fd = open(dev)
>> 2. ioctl(fd, claim host)
>> 3. read(fd, ...)
>>
>> (if 3. is not convincing, replace it with any access to mounted
>> filesystem from the same card.)
>
> I agree. If we allow such a command on a block device, that
> should probably be mutually exclusive with normal read/write
> access or mounting the block device, and it needs to be a
> priviledged operation.
>

Chances are, the filesystem could be located on the device itself, so
exclusive access isn't always possible. Right now you can 'dd' all
over a block device
even if a filesystem is mounted on it, so the ioctl shouldn't be any
more restrictive than that. The contract on the ioctl should be that
the rest of the sd/sdio stack is not confused
over the card state, possibly resulting in the wrong operation. For
example, I briefly skimmed the simplified SD physical layer spec, and
I found nothing that said it was safe to assume a particular command
set would always work while the card is in some particular function
mode (set using SWITCH command, for example to put card into OTP,
ASSD.) In fact, if you look at 4.3.12 of
Part_1_Physical_Layer_Simplified_Specification_Ver3.01_Final_100518.pdf,
it says right there that care must be shown as to what command system
mode the card is in, as the commands may change meanings.

I think you need to nail down what exactly is going to be sent across
the IOCTL within the boundary of the simplified SD spec. If the
security extensions involve putting the card into a mode where normal
block/sdio/whatever functionality is impaired until the mode is
exited, then the kernel should always have the ability to put the card
back in normal mode, and restore your special mode when you do another
related ioctl.

A

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMD

[John Calixto]

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

On Mon, 21 Mar 2011, Andrei Warkentin wrote:

> 2011/3/21 Arnd Bergmann <arnd@arndb.de>:
> > On Sunday 20 March 2011, Michał Mirosław wrote:
> >> 2011/3/20 John Calixto <john.calixto@modsystems.com>:
> >> > On Sat, 19 Mar 2011, Arnd Bergmann wrote:
> >> [...]
> >> >> > I expect for more general purposes, like your qemu passthrough case, you
> >> >> > would want to add a handler for cmd=SD_IOC_CMD?
> >> >> Yes, but I suspect that would mean blocking all other activity on the
> >> >> same device, possibly with an ioctl for claiming the host for an
> >> >> extended period of time.
> >> > Sure.  And if the testing I described above shows that regular block
> >> > operations are indeed negatively affected, then I'll have to implement
> >> > that ioctl for claiming the host myself and you'll have a head start! :)
> >>
> >> This idea is scary. ;-) Just think what would happen if userspace made
> >> this sequence:
> >> 1. fd = open(dev)
> >> 2. ioctl(fd, claim host)
> >> 3. read(fd, ...)
> >>
> >> (if 3. is not convincing, replace it with any access to mounted
> >> filesystem from the same card.)
> >
> > I agree. If we allow such a command on a block device, that
> > should probably be mutually exclusive with normal read/write
> > access or mounting the block device, and it needs to be a
> > priviledged operation.
> >
> 
> Chances are, the filesystem could be located on the device itself, so
> exclusive access isn't always possible. Right now you can 'dd' all
> over a block device
> even if a filesystem is mounted on it, so the ioctl shouldn't be any
> more restrictive than that. The contract on the ioctl should be that
> the rest of the sd/sdio stack is not confused
> over the card state, possibly resulting in the wrong operation. For
> example, I briefly skimmed the simplified SD physical layer spec, and
> I found nothing that said it was safe to assume a particular command
> set would always work while the card is in some particular function
> mode (set using SWITCH command, for example to put card into OTP,
> ASSD.) In fact, if you look at 4.3.12 of
> Part_1_Physical_Layer_Simplified_Specification_Ver3.01_Final_100518.pdf,
> it says right there that care must be shown as to what command system
> mode the card is in, as the commands may change meanings.
> 
> I think you need to nail down what exactly is going to be sent across
> the IOCTL within the boundary of the simplified SD spec. If the
> security extensions involve putting the card into a mode where normal
> block/sdio/whatever functionality is impaired until the mode is
> exited, then the kernel should always have the ability to put the card
> back in normal mode, and restore your special mode when you do another
> related ioctl.
> 

I think this is headed in the right direction.  Instead of creating
special cases for specific commands, I'd prefer to make the default case
be that the kernel should always be able to use the SD card as it
expects for normal storage operations.  Something to the effect of:

<pseudo>

    static int ioctl_expect_more = 0;  /* when true, some caller expects to send multiple cmds via ioctl */
    static int ioctl_cmd_source = 0;  /* by default, cmds do *not* come from ioctl */

    static int mmc_blk_ioctl(bdev, mode, cmd, arg)
    {

        ...

        mmc_claim_host(host);
        ioctl_cmd_source = 1;

        ioctl_expect_more = arg.expect_more;
        if (ioctl_expect_more) {
            save_preioctl_sd_state();
        }

        mmc_wait_for_req(host, mrq);

        ioctl_cmd_source = 0;
        mmc_release_host(host);

        ...
    }

    void mmc_wait_for_req(host, mrq)
    {

        ...

        if (ioctl_expect_more && !ioctl_cmd_source) {
            restore_preioctl_sd_state();
            ioctl_expect_more = 0;
        }

        ...

    }

</pseudo>

Secure ops, virtualization, and really anything that would use this
ioctl do not fit the 'normal' path for most users.  I think it's
appropriate to expect the applications that use this ioctl to be able to
handle interruptions in their SD command sequences.  I know that I
already need to be careful about this for secure ops.  And I would hope
that if you were using this ioctl to passthrough SD storage to your
virtualized guests, you would not expect it to be used concurrently as
the storage for your host rootfs as well.

John

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs

[John Calixto]

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

On Sat, 19 Mar 2011, Michał Mirosław wrote:
> W dniu 18 marca 2011 20:26 użytkownik Arnd Bergmann <arnd@arndb.de> napisał:
> > On Friday 18 March 2011 18:56:53 Michał Mirosław wrote:
> >> If that's going to be used by possibly unprivileged userspace process,
> >> then this passthrough should filter and validate all commands it
> >> passes to hardware. If there is a possibility of some command sequence
> >> to generate undefined or otherwise unwanted results, then you need
> >> state tracker that will disallow that sequence to be generated by
> >> unprivileged process.
> > We have precedence for direct host commands in a few other
> > block drivers. In general, any user who can open the block
> > device can issue all commands unless they can directly destroy
> > the hardware. On normal systems, the only user that has write
> > access to block devices is root.
> 
> In this case, a process having access to one partition can disrupt
> other partitions on the same card even if it has no access to them in
> any other way.

This is true, but I can already wreak havoc on partitions for any block
device by opening up the block device node directly, seeking and
writing.  If I have write access to the block device, I can do whatever
I want.

> 
> It is not that unusual on "normal systems" to give write access to
> some partition or device to unprivileged users. Database volumes are
> one example.
> 

Are you talking about the device nodes themselves, or about access to
the mounted filesystems that live on those devices/partitions?  It seems
like if you give unprivileged users write access to the raw block
device, you should expect a lot more trouble from
runaway/malicious/accidental writes than you would from
application-specific commands being sent via ioctl.

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs

[Michał Mirosław]

W dniu 21 marca 2011 19:37 użytkownik John Calixto
<john.calixto@modsystems.com> napisał:
> On Sat, 19 Mar 2011, Michał Mirosław wrote:
>> W dniu 18 marca 2011 20:26 użytkownik Arnd Bergmann <arnd@arndb.de> napisał:
>> > On Friday 18 March 2011 18:56:53 Michał Mirosław wrote:
>> >> If that's going to be used by possibly unprivileged userspace process,
>> >> then this passthrough should filter and validate all commands it
>> >> passes to hardware. If there is a possibility of some command sequence
>> >> to generate undefined or otherwise unwanted results, then you need
>> >> state tracker that will disallow that sequence to be generated by
>> >> unprivileged process.
>> > We have precedence for direct host commands in a few other
>> > block drivers. In general, any user who can open the block
>> > device can issue all commands unless they can directly destroy
>> > the hardware. On normal systems, the only user that has write
>> > access to block devices is root.
>> In this case, a process having access to one partition can disrupt
>> other partitions on the same card even if it has no access to them in
>> any other way.
> This is true, but I can already wreak havoc on partitions for any block
> device by opening up the block device node directly, seeking and
> writing.  If I have write access to the block device, I can do whatever
> I want.

You're talking about reverse case to what I described. Using a
partition, you shouldn't be able to effect the device in a way that
extends part this particular partition.

>> It is not that unusual on "normal systems" to give write access to
>> some partition or device to unprivileged users. Database volumes are
>> one example.
> Are you talking about the device nodes themselves, or about access to
> the mounted filesystems that live on those devices/partitions?  It seems
> like if you give unprivileged users write access to the raw block
> device, you should expect a lot more trouble from
> runaway/malicious/accidental writes than you would from
> application-specific commands being sent via ioctl.

I don't exactly see what's your point here. If you say that writes are
less dangerous than ioctls, then I agree. Even now, for some block
ioctls you need CAP_SYS_ADMIN because of that.

Best Regards,
Michał Mirosław

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs

[John Calixto]

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

On Tue, 22 Mar 2011, Michał Mirosław wrote:
> >> In this case, a process having access to one partition can disrupt
> >> other partitions on the same card even if it has no access to them in
> >> any other way.
> > This is true, but I can already wreak havoc on partitions for any block
> > device by opening up the block device node directly, seeking and
> > writing.  If I have write access to the block device, I can do whatever
> > I want.
> 
> You're talking about reverse case to what I described. Using a
> partition, you shouldn't be able to effect the device in a way that
> extends part this particular partition.
> 

Ah - I see.  Sorry, I misread your comment.

> >> It is not that unusual on "normal systems" to give write access to
> >> some partition or device to unprivileged users. Database volumes are
> >> one example.
> > Are you talking about the device nodes themselves, or about access to
> > the mounted filesystems that live on those devices/partitions?  It seems
> > like if you give unprivileged users write access to the raw block
> > device, you should expect a lot more trouble from
> > runaway/malicious/accidental writes than you would from
> > application-specific commands being sent via ioctl.
> 
> I don't exactly see what's your point here. If you say that writes are
> less dangerous than ioctls, then I agree. Even now, for some block
> ioctls you need CAP_SYS_ADMIN because of that.
> 

In general, I agree with your caution.  My point is that I'm not sure
this type of protection belongs in the kernel.  We use permissions and
"medium-privileged" role users all the time to marshal access to
sensitive files and devices.  This is a problem that has been solved in
userspace.  You don't let normal user "john" have the same access to
device nodes or even files and directories that you would for role user
"database", or role user "webserver".  (Actually, you probably shouldn't
let normal user "john" have access to anything - I hear he's trouble!)

John

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs

[Michał Mirosław]

W dniu 22 marca 2011 23:31 użytkownik John Calixto
<john.calixto@modsystems.com> napisał:
> On Tue, 22 Mar 2011, Michał Mirosław wrote:
>> >> It is not that unusual on "normal systems" to give write access to
>> >> some partition or device to unprivileged users. Database volumes are
>> >> one example.
>> > Are you talking about the device nodes themselves, or about access to
>> > the mounted filesystems that live on those devices/partitions?  It seems
>> > like if you give unprivileged users write access to the raw block
>> > device, you should expect a lot more trouble from
>> > runaway/malicious/accidental writes than you would from
>> > application-specific commands being sent via ioctl.
>> I don't exactly see what's your point here. If you say that writes are
>> less dangerous than ioctls, then I agree. Even now, for some block
>> ioctls you need CAP_SYS_ADMIN because of that.
> In general, I agree with your caution.  My point is that I'm not sure
> this type of protection belongs in the kernel.  We use permissions and
> "medium-privileged" role users all the time to marshal access to
> sensitive files and devices.  This is a problem that has been solved in
> userspace.  You don't let normal user "john" have the same access to
> device nodes or even files and directories that you would for role user
> "database", or role user "webserver".  (Actually, you probably shouldn't
> let normal user "john" have access to anything - I hear he's trouble!)

When you grant write access to a device to some user, you should
expect that it is all you are granting. There shouldn't be any hidden
doors that, for example, if underlying device is SD card then you can
destroy it by this ioctl(). Not counting wearing or WORM-like media,
writes (also erasing, changing encryption keys, etc.) are undoable.
Other forms of access should be granted separately (by capabilities or
other means).

Best Regards,
Michał Mirosław

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs

[John Calixto]

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

On Wed, 23 Mar 2011, Michał Mirosław wrote:
> When you grant write access to a device to some user, you should
> expect that it is all you are granting. There shouldn't be any hidden
> doors that, for example, if underlying device is SD card then you can
> destroy it by this ioctl(). Not counting wearing or WORM-like media,
> writes (also erasing, changing encryption keys, etc.) are undoable.
> Other forms of access should be granted separately (by capabilities or
> other means).
> 

Fair enough.  I'm not aware enough of the other ACMDs that might
actually destroy the card (nothing I'm using will destroy the card), so
I'll be sure to hook it with CAP_SYS_ADMIN (or whatever capability is
most appropriate).

John

Re: [PATCH resend] mmc: Added ioctl to let userspace apps send ACMDs

[Arnd Bergmann]

On Wednesday 23 March 2011, John Calixto wrote:
> 
> On Wed, 23 Mar 2011, Michał Mirosław wrote:
> > When you grant write access to a device to some user, you should
> > expect that it is all you are granting. There shouldn't be any hidden
> > doors that, for example, if underlying device is SD card then you can
> > destroy it by this ioctl(). Not counting wearing or WORM-like media,
> > writes (also erasing, changing encryption keys, etc.) are undoable.
> > Other forms of access should be granted separately (by capabilities or
> > other means).
> > 
> 
> Fair enough.  I'm not aware enough of the other ACMDs that might
> actually destroy the card (nothing I'm using will destroy the card), so
> I'll be sure to hook it with CAP_SYS_ADMIN (or whatever capability is
> most appropriate).

The standard defines some commands as vendor-specific. A typical use
case for these would be a way to update the firmware on the embedded
microcontroller of the card.

Overwriting that firmware with garbage would be an obvious way to
brick the card.

	Arnd