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 */ > >