On Wed, 6 Apr 2011, Michał Mirosław wrote:
> 2011/4/5 John Calixto <john.cali...@modsystems.com>:
> > + /*
> > + * Only allow ACMDs on the whole block device, not on partitions.
> > This
> > + * prevents overspray between sibling partitions.
> > + */
> > + if (bdev != bdev->bd_contains)
> > + return -EPERM;
>
> This should at least also check CAP_SYS_ADMIN capability.
>
Yep. I broke out the capability check into a separate patch - I'll
reply to your permissions-related comments on that thread.
> > +
> > + md = mmc_blk_get(bdev->bd_disk);
> > + if (!md)
> > + return -EINVAL;
> > +
> > + card = md->queue.card;
> > + if (IS_ERR(card))
> > + return PTR_ERR(card);
> > +
> > + host = card->host;
> > + mmc_claim_host(host);
> > +
> > + err = mmc_app_cmd(host, card);
> > + if (err)
> > + goto acmd_done;
> > +
> > + mrq.cmd = &cmd;
> > + mrq.data = &data;
> > +
> > + if (copy_from_user(&sdic, sdic_ptr, sizeof(sdic))) {
> > + err = -EFAULT;
> > + goto acmd_done;
> > + }
>
> You should first copy and verify ioctl's data and only then claim host
> and send commands. Preferably the check-and-copy should be separate
> function.
>
Will do.
> > +
> > +#ifdef CONFIG_COMPAT
> > +struct sd_ioc_cmd32 {
> > + u32 write_flag;
> > + u32 opcode;
> > + u32 arg;
> > + u32 response[4];
> > + u32 flags;
> > + u32 blksz;
> > + u32 blocks;
> > + u32 postsleep_us;
> > + u32 force_timeout_ns;
> > + compat_uptr_t data_ptr;
> > +};
> > +#define SD_IOC_ACMD32 _IOWR(MMC_BLOCK_MAJOR, 0, struct sd_ioc_cmd32)
> [...]
>
> Since your implementing a new ioctl you can make the structure the
> same for 64 and 32-bit archs and avoid all this compat crap.
>
I was also less than pleased with this, but I chose to implement the
compat crap because it allow a "natural" userspace API for both the
kernel32+user32 and kernel64+user64 environments. The ugliness in the
kernel is just when you have defined CONFIG_COMPAT. I think 32-bit-only
is still an important target for this functionality (think set-top media
players, mobile devices; basically, anything running on ARM) so always
having to cast your data pointer to 64-bit is not appealing. I suspect
it will be very unlikely to see people using this in the kernel64+user32
environment.
Cheers,
John
