On Thursday 11 February 2016 06:09:13 Soohoon Lee wrote:
>
> thanks for your reply.
> It works as expected.
> is it going to be submitted?
Yes, I can submit that as a proper patch, thanks for testing!
Arnd
> On Tuesday 09 February 2016 17:38:56 Soohoon Lee wrote:
> >
> > Hi,
> > I found that you are the author of this code.
>
> I don't think I am, but that's fine. I think I just moved the code
> from one place to another.
>
> > So please let me ask a question.
>
> I hope it's ok for you to Cc this to a linux-ide and linux-kernel.
>
> > +static int compat_hdio_ioctl(struct inode *inode, struct file *file,
> > + struct gendisk *disk, unsigned int cmd, unsigned long arg)
> > +{
> > + mm_segment_t old_fs = get_fs();
> > + unsigned long kval;
> > + unsigned int __user *uvp;
> > + int error;
> > +
> > + set_fs(KERNEL_DS);
> > + error = blkdev_driver_ioctl(inode, file, disk,
> > + cmd, (unsigned long)(&kval));
> > + set_fs(old_fs);
> > +
> > + if (error == 0) {
> > + uvp = compat_ptr(arg);
> > + if (put_user(kval, uvp))
> > + error = -EFAULT;
> > + }
> > + return error;
> > +}
> >
> >
> > kval is local so it has random values.
> > But one of syscall like HDIO_GET_32BIT/ATA_IOC_GET_IO32 only updates one byte so if kval has 0xaaaaaaaa and ioctl() updated one byte then it becomes 0xaaaaaa01.
> > And put_user() writes 4bytes.
>
> This actually looks like a security bug, as we are not supposed to leak
> kernel stack data.
>
> > And I'm having problem with hdparm.
> >
> > void process_dev (char *devname)
> > {
> > static long parm, multcount; <-- parm is static so it's zero
> > .....
> > if (do_defaults || get_io32bit) {
> > if (0 == ioctl(fd, HDIO_GET_32BIT, &parm)) {
> >
> >
> > so parm becomes 0xaaaaaa01 and reports wrong mode.
> >
> > If hdparm is 64bit then 64bit syscall will update 1byte of parm so it becomes 0x1.
> >
> > What would be a good fix?
> >
> > - Modify hdparm to look at only 1byte
> >
> > - Initialize kval to zero
> >
> > - Copy parm to kval to simulate 64bit syscall more accurately.
> >
>
> Out of these, the last one is the best. My preferred solution however would
> be to move the handling of the ioctls in question (and maybe some others
> while we're at it) into the driver that provides the call in the first place.
>
> There is another problem: I don't think the code ever worked on big-endian
> machines:
>
> case ATA_IOC_GET_IO32:
> spin_lock_irqsave(ap->lock, flags);
> val = ata_ioc32(ap);
> spin_unlock_irqrestore(ap->lock, flags);
> if (copy_to_user(arg, &val, 1))
> return -EFAULT;
> return 0;
>
>
> On little-endian machines, this copies the low byte of the 'int val'
> variable, while on big-endian machines, it copies the high byte that
> is always zero. In the hdparm source code, this gets copied into a
> 'static long parm' variable, which in turn means that when we first read
> one 32-bit setting and then call ATA_IOC_GET_IO32 as part of the same
> hdparm command line, the upper 24 or 56 bits (out of 32 or 64 respectively)
> still contain the previous result.
>
> From what I can tell, the behavior of hdparm matches the behavior of
> the old drivers/ide/ subsystem, and the compat ioctl handling works with
> that, but the ioctls in drivers/ata/ suffer from both the compat_ioctl
> problem and the problem on big-endian systems.
>
> Both problems date back to the original commit that added ioctl support
> in libata back in 2004 (linux-2.6.8).
> Can you try out the patch below to see if that makes it work? If it
> does, we probably want something like this backported to all stable
> kernels, but I think we probably also want to clean this up a bit
> more to avoid the 'get_fs()/set_fs()' hack in block/compat_ioctl.c.
>
> Arnd
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 7e959f90c020..e417e1a1d02c 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -675,19 +675,18 @@ static int ata_ioc32(struct ata_port *ap)
> int ata_sas_scsi_ioctl(struct ata_port *ap, struct scsi_device *scsidev,
> int cmd, void __user *arg)
> {
> - int val = -EINVAL, rc = -EINVAL;
> + unsigned long val;
> + int rc = -EINVAL;
> unsigned long flags;
>
> switch (cmd) {
> - case ATA_IOC_GET_IO32:
> + case HDIO_GET_32BIT:
> spin_lock_irqsave(ap->lock, flags);
> val = ata_ioc32(ap);
> spin_unlock_irqrestore(ap->lock, flags);
> - if (copy_to_user(arg, &val, 1))
> - return -EFAULT;
> - return 0;
> + return put_user(val, (unsigned long __user *)arg);
>
> - case ATA_IOC_SET_IO32:
> + case HDIO_SET_32BIT:
> val = (unsigned long) arg;
> rc = 0;
> spin_lock_irqsave(ap->lock, flags);
> diff --git a/include/linux/ata.h b/include/linux/ata.h
> index d2992bfa1706..c1a2f345cbe6 100644
> --- a/include/linux/ata.h
> +++ b/include/linux/ata.h
> @@ -487,8 +487,8 @@ enum ata_tf_protocols {
> };
>
> enum ata_ioctls {
> - ATA_IOC_GET_IO32 = 0x309,
> - ATA_IOC_SET_IO32 = 0x324,
> + ATA_IOC_GET_IO32 = 0x309, /* HDIO_GET_32BIT */
> + ATA_IOC_SET_IO32 = 0x324, /* HDIO_SET_32BIT */
> };
>
> /* core structures */
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
