Re: compat_hdio_ioctl() question

Re: compat_hdio_ioctl() question

[Arnd Bergmann]

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

Re: compat_hdio_ioctl() question

[Soohoon Lee]

thanks for your reply.
It works as expected.
is it going to be submitted?

Thanks,
Soohoon.

________________________________________
From: Arnd Bergmann <arnd@arndb.de>
Sent: Wednesday, February 10, 2016 4:55 PM
To: Soohoon Lee
Cc: Mark Lord; linux-ide@vger.kernel.org; Tejun Heo; linux-kernel@vger.kernel.org
Subject: Re: compat_hdio_ioctl() question

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

Re: compat_hdio_ioctl() question

[Arnd Bergmann]

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