On Wed, 6 Apr 2011, MichaÅ MirosÅaw wrote:
> 2011/4/5 John Calixto <john.calixto@xxxxxxxxxxxxxx>:
> > + Â Â Â /*
> > + Â Â Â Â* 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
