| Message ID | alpine.DEB.2.00.1104211743210.3919@peruna (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 21 Apr 2011, John Calixto wrote: <snip> > +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 = (u64) idata->ic.blksz * idata->ic.blocks; > + if (idata->buf_bytes > MMC_IOC_MAX_BYTES) { > + err = -EOVERFLOW; > + goto copy_err; > + } > + > + idata->buf = kzalloc(idata->buf_bytes, GFP_KERNEL); > + if (!idata->buf) { > + err = -ENOMEM; > + goto copy_err; > + } > + Per Arnd's recommendation, I just cast the ``data_ptr`` to ``(void *)(unsigned long)`` to solve the compatibility issue with the buffer pointer in struct mmc_ioc_cmd. > + if (copy_from_user(idata->buf, (void __user *)(unsigned long) > + idata->ic.data_ptr, idata->buf_bytes)) { > + err = -EFAULT; > + goto copy_err; > + } > + > + return idata; > + > +copy_err: > + kfree(idata->buf); > + kfree(idata); > + return ERR_PTR(err); > + > +} > + <snip> I also left mmc_blk_ioctl() and mmc_blk_compat_ioctl() as they were in v6. IMHO, a __mmc_blk_ioctl() function that accepts a compat32 boolean arg does not do much to enhance readability or functionality in this case. With the implementation in the 2 functions below, I intended to make it clear that: - mmc_blk_ioctl() is responsible for delegating to the appropriate handler based on the incoming ioctl cmd. The ``if`` would be replaced with a ``switch`` when adding other values for cmd. - mmc_blk_compat_ioctl() is responsible for converting the ioctl arg to something suitable for consumtption by mmc_blk_ioctl(). - when CONFIG_COMPAT is undefined, you get no code related to compatibility. > + > +static int mmc_blk_ioctl(struct block_device *bdev, fmode_t mode, > + unsigned int cmd, unsigned long arg) > +{ > + int ret = -EINVAL; > + if (cmd == MMC_IOC_CMD) > + ret = mmc_blk_ioctl_cmd(bdev, (struct mmc_ioc_cmd __user *)arg); > + return ret; > +} > + > +#ifdef CONFIG_COMPAT > +static int mmc_blk_compat_ioctl(struct block_device *bdev, fmode_t mode, > + unsigned int cmd, unsigned long arg) > +{ > + return mmc_blk_ioctl(bdev, mode, cmd, (unsigned long) compat_ptr(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 > }; > <snip> It makes a lot of sense to me to make sure that struct mmc_ioc_cmd is the same size on 32-bit and 64-bit machines, since it is used by _IOWR() to derive the value for MMC_IOC_CMD. Any change in its size requires having other compatibility logic to deal with a MMC_IOC_CMD32. Note that as this struct has evolved, I also needed to add a ``__pad`` member to the struct to achieve the goal of keeping it the same size on 32-bit and 64-bit. I could have used packing or alignment compiler directives, but this seemed simplest to me (I can add up the bytes in my head!): > diff --git a/include/linux/mmc/ioctl.h b/include/linux/mmc/ioctl.h > new file mode 100644 > index 0000000..225e1e1 > --- /dev/null > +++ b/include/linux/mmc/ioctl.h > @@ -0,0 +1,54 @@ > +#ifndef LINUX_MMC_IOCTL_H > +#define LINUX_MMC_IOCTL_H > +struct mmc_ioc_cmd { > + /* Implies direction of data. true = write, false = read */ > + int write_flag; > + > + /* Application-specific command. true = precede with CMD55 */ > + int is_acmd; > + > + __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; > + > + /* > + * For 64-bit machines, the next member, ``__u64 data_ptr``, wants to > + * be 8-byte aligned. Make sure this struct is the same size when > + * built for 32-bit. > + */ > + __u32 __pad; > + > + /* DAT buffer */ > + __u64 data_ptr; > +}; To help with the casting in 32-bit userspace, I also provided ``mmc_ioc_cmd_set_data(ic, ptr)`` as a helper macro: > +#define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr > + > +#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd) > + > +/* > + * Since this ioctl is only meant to enhance (and not replace) normal access to > + * the mmc bus device, an upper data transfer limit of MMC_IOC_MAX_BYTES is > + * enforced per ioctl call. For larger data transfers, use the normal block > + * device operations. > + */ > +#define MMC_IOC_MAX_BYTES (512L * 256) > +#endif /* LINUX_MMC_IOCTL_H */ > -- > 1.7.4.1 > John -- 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
On Friday 22 April 2011, John Calixto wrote: > Allows appropriately-privileged applications to send CMD (normal) and > ACMD (application-specific; preceded with CMD55) commands to > cards/devices on the mmc bus. This is primarily useful for enabling the > security functionality built in to every SD card. > > It can also be used as a generic passthrough (e.g. to enable virtual > machines to control mmc bus devices directly). However, this use case > has not been tested rigorously. Generic passthrough testing was only > conducted for a few non-security opcodes to prove the feasibility of the > passthrough. > > Since any opcode can be sent using this passthrough, it is very possible > to render the card/device unusable. Applications that use this ioctl > must have CAP_SYS_RAWIO. > > Security commands 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> The implementation looks good to me now, Reviewed-by: Arnd Bergmann <arnd@arndb.de> I'll leave the final decision whether this is a good feature to have to Chris. I still believe that we should have per-command ioctls for the security feature, but getting there would require someone to implement it, and I'm not going to do that. Arnd -- 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
On Tue, 26 Apr 2011, Arnd Bergmann wrote: > On Friday 22 April 2011, John Calixto wrote: > > Allows appropriately-privileged applications to send CMD (normal) and > > ACMD (application-specific; preceded with CMD55) commands to > > cards/devices on the mmc bus. This is primarily useful for enabling the > > security functionality built in to every SD card. > > > > It can also be used as a generic passthrough (e.g. to enable virtual > > machines to control mmc bus devices directly). However, this use case > > has not been tested rigorously. Generic passthrough testing was only > > conducted for a few non-security opcodes to prove the feasibility of the > > passthrough. > > > > Since any opcode can be sent using this passthrough, it is very possible > > to render the card/device unusable. Applications that use this ioctl > > must have CAP_SYS_RAWIO. > > > > Security commands 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> > > The implementation looks good to me now, > > Reviewed-by: Arnd Bergmann <arnd@arndb.de> > > I'll leave the final decision whether this is a good feature to have > to Chris. I still believe that we should have per-command ioctls > for the security feature, but getting there would require someone > to implement it, and I'm not going to do that. > Arnd - Thanks a lot for the review and your help getting the implementation right! Chris - What do you think? How should I proceed here? John -- 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
Hi John, On Tue, Apr 26 2011, John Calixto wrote: > Arnd - Thanks a lot for the review and your help getting the > implementation right! > > Chris - What do you think? How should I proceed here? I think I'll merge it, since I'm not going to work on abstracting the security feature either. I'll send another mail once it's in my tree. Thanks! - Chris.
On Tue, 26 Apr 2011, Chris Ball wrote: > Hi John, > > On Tue, Apr 26 2011, John Calixto wrote: > > Arnd - Thanks a lot for the review and your help getting the > > implementation right! > > > > Chris - What do you think? How should I proceed here? > > I think I'll merge it, since I'm not going to work on abstracting the > security feature either. I'll send another mail once it's in my tree. > > Thanks! > > - Chris. Chris, That's great - thanks a lot! John -- 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
Hi John, On Tue, Apr 26 2011, Chris Ball wrote: > I think I'll merge it, since I'm not going to work on abstracting the > security feature either. I'll send another mail once it's in my tree. I've pushed this out to mmc-next now. - Chris.
diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt index a0a5d82..2a34d82 100644 --- a/Documentation/ioctl/ioctl-number.txt +++ b/Documentation/ioctl/ioctl-number.txt @@ -304,6 +304,7 @@ Code Seq#(hex) Include File Comments 0xB0 all RATIO devices in development: <mailto:vgo@ratio.de> 0xB1 00-1F PPPoX <mailto:mostrows@styx.uwaterloo.ca> +0xB3 00 linux/mmc/ioctl.h 0xC0 00-0F linux/usb/iowarrior.h 0xCB 00-1F CBM serial IEC bus in development: <mailto:michael.klein@puffin.lb.shuttle.de> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c index 61d233a..758419a 100644 --- a/drivers/mmc/card/block.c +++ b/drivers/mmc/card/block.c @@ -31,7 +31,11 @@ #include <linux/mutex.h> #include <linux/scatterlist.h> #include <linux/string_helpers.h> +#include <linux/delay.h> +#include <linux/capability.h> +#include <linux/compat.h> +#include <linux/mmc/ioctl.h> #include <linux/mmc/card.h> #include <linux/mmc/host.h> #include <linux/mmc/mmc.h> @@ -158,11 +162,208 @@ 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; + u64 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 = (u64) idata->ic.blksz * idata->ic.blocks; + if (idata->buf_bytes > MMC_IOC_MAX_BYTES) { + err = -EOVERFLOW; + goto copy_err; + } + + idata->buf = kzalloc(idata->buf_bytes, GFP_KERNEL); + if (!idata->buf) { + err = -ENOMEM; + goto copy_err; + } + + if (copy_from_user(idata->buf, (void __user *)(unsigned long) + idata->ic.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_cmd(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 cmd_done; + } + + card = md->queue.card; + if (IS_ERR(card)) { + err = PTR_ERR(card); + goto cmd_done; + } + + mmc_claim_host(card->host); + + if (idata->ic.is_acmd) { + err = mmc_app_cmd(card->host, card); + if (err) + goto cmd_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 cmd_rel_host; + } + if (data.error) { + dev_err(mmc_dev(card->host), "%s: data error %d\n", + __func__, data.error); + err = data.error; + goto cmd_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 cmd_rel_host; + } + + if (!idata->ic.write_flag) { + if (copy_to_user((void __user *)(unsigned long) idata->ic.data_ptr, + idata->buf, idata->buf_bytes)) { + err = -EFAULT; + goto cmd_rel_host; + } + } + +cmd_rel_host: + mmc_release_host(card->host); + +cmd_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; + if (cmd == MMC_IOC_CMD) + ret = mmc_blk_ioctl_cmd(bdev, (struct mmc_ioc_cmd __user *)arg); + return ret; +} + +#ifdef CONFIG_COMPAT +static int mmc_blk_compat_ioctl(struct block_device *bdev, fmode_t mode, + unsigned int cmd, unsigned long arg) +{ + return mmc_blk_ioctl(bdev, mode, cmd, (unsigned long) compat_ptr(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..225e1e1 --- /dev/null +++ b/include/linux/mmc/ioctl.h @@ -0,0 +1,54 @@ +#ifndef LINUX_MMC_IOCTL_H +#define LINUX_MMC_IOCTL_H +struct mmc_ioc_cmd { + /* Implies direction of data. true = write, false = read */ + int write_flag; + + /* Application-specific command. true = precede with CMD55 */ + int is_acmd; + + __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; + + /* + * For 64-bit machines, the next member, ``__u64 data_ptr``, wants to + * be 8-byte aligned. Make sure this struct is the same size when + * built for 32-bit. + */ + __u32 __pad; + + /* DAT buffer */ + __u64 data_ptr; +}; +#define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr + +#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd) + +/* + * Since this ioctl is only meant to enhance (and not replace) normal access to + * the mmc bus device, an upper data transfer limit of MMC_IOC_MAX_BYTES is + * enforced per ioctl call. For larger data transfers, use the normal block + * device operations. + */ +#define MMC_IOC_MAX_BYTES (512L * 256) +#endif /* LINUX_MMC_IOCTL_H */