Re: [PATCH] 2.5.24 IDE 95 (fwd)

Re: [PATCH] 2.5.24 IDE 95 (fwd)

[Bartlomiej Zolnierkiewicz]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1811 bytes --]


I hope you dont mind Petr.

---------- Forwarded message ----------
Date: Sun, 30 Jun 2002 18:20:03 +0200 (MET DST)
From: Bartlomiej Zolnierkiewicz <bzolnier@elka.pw.edu.pl>
To: Petr Vandrovec <vandrove@vc.cvut.cz>
Subject: Re: [PATCH] 2.5.24 IDE 95


Hi!

On Sun, 30 Jun 2002, Petr Vandrovec wrote:

> On Sun, Jun 30, 2002 at 12:53:57AM +0200, Bartlomiej Zolnierkiewicz wrote:
> >
> > Hi Petr!
> >
> > On Sun, 30 Jun 2002, Petr Vandrovec wrote:
> >
> > > Hi,
> > >   I know that IDE95 is broken when it comes to IDE, but... not only
>
> Oops. It should read ... when it comes to CD, ...
> Thanks for your updates, applying them just now.
> 							Petr
>
> P.S.: Locking looks suspicious too. On other side, if hardware is broken
> and requires large data transfers during interrupt, why we should
> bother? ne2000 driver always required disabling interrupts for up to 1.6ms...

1.6ms really bad, masked PIO is about 2ms, this can affect audio etc.,
Legacy/special/broken hardware does only PIO, but most of it do not
require transfers with IRQs disabled (thats why ch->unmask flag).

Even with new locking (ch->lock for everything) we don't have to disable
IRQs completly, we can mask our IRQ and enable rest, but it is not
possible to i.e. share IRQ with audio (because we disable our IRQ).
It was (at least in theory) possible with old locking.

Old locking is that ch->lock spinlock protects access to IDE_BUSY bit in
ch->active, ch->handler and ch->timer. IDE_BUSY protects hardware access,
if it is set we do not enter start_request() in do_request(). When we start
request block layer sets RQ_STARTED flag on request so it is protected
from block layer side. Thats almost all about locking.

Hope this clarifies some things, also old akpm's note about IDE
unmaskirqs attached....

Greets.
--
Bartlomiej

[-- Attachment #2: Type: TEXT/PLAIN, Size: 2441 bytes --]

Date: Sat, 22 Apr 2000 01:51:54 +1000
From: Andrew Morton <andrewm@uow.edu.au>
Subject: Re: IDE drives and unmaskirq

[ I cut lkml - I've been posting too much today :) ]

Martijn van Oosterhout wrote:
> 
> Odd, both my hard disks are UDMA. You're saying unmaskirq has
> no effect on UDMA systems?

I have a patch which measures interrupt latencies.  It tells you how
long the kernel spends with interrupts disabled: where they were turned
off, where they were turned on, min, max, avg number of microseconds.

With UDMA66 drives:

hdparm -u0 -d0  (interrupts masked, PIO)

                                   Count   Min      Max      Avg
ide-disk.c:451 -> ll_rw_blk.c:277  12     825.17  2350.14  1268.35
     ide-disk.c:451 -> ide.c:1620  591    818.82  2172.18  1138.26
         ide.c:1531 -> ide.c:1620  7407    12.13  2147.32  1519.29
         ide.c:1531 -> ide.c:1296  721      9.51  1930.11   236.73
    ll_rw_blk.c:271 -> ide.c:1296  2912    13.82    27.89    16.56
         ...

hdparm -t says 4.1 MB/sec

(The first traversal seems a very odd place to reenable...)

-------

hdparm -u0 -d1 (interrupts masked, DMA)

     ide.c:1531 -> ide.c:1620  5230     11.13    90.76    26.55
     ide.c:1531 -> ide.c:1296  484      11.53    73.15    17.15
ll_rw_blk.c:271 -> ide.c:1296  5232     13.79    28.07    16.51
ide.c:1298 -> ll_rw_blk.c:277  5232      7.33    14.40     8.23
      ...

hdparm -t says 21 MB/sec

------

hdparm -u1 -d0 (interrupts unmasked, PIO)

          ide.c:522 -> ide.c:531  8631     .81    32.30     1.93
   ll_rw_blk.c:271 -> ide.c:1296  3921   13.82    28.94    16.52
          ide.c:497 -> ide.c:506  14388   1.07    24.79     6.07
   ide.c:1298 -> ll_rw_blk.c:277  3921    7.33    14.76     8.15
         ...

hdparm -t says 4.2 MB/sec

--------

hdparm -u1 -d1 (interrupts unmasked, DMA)

ll_rw_blk.c:271 -> ide.c:1296  3950     13.78    26.74    16.43
     ide.c:1713 -> ide.c:1296  1        23.61    23.61    23.61
       ide.c:497 -> ide.c:506  14623      .99    23.22     5.06
ide.c:1298 -> ll_rw_blk.c:277  3950      7.35    15.72     8.11
         ...

hdparm -t says 22 MB/sec


So you can see that even with DMA, 'hdparm -u1' cuts the max
interrupt-disable time from 90 to 27 microseconds.  I somehow doubt if
that will affect interactive perceptions, but the two milliseconds for
PIO/masked will affect audio, for example.

HDIO_GETGEO accessibility (was Re: [PATCH] 2.5.24 IDE 95 (fwd))

[Petr Vandrovec]

On Sun, Jun 30, 2002 at 06:52:58PM +0200, Bartlomiej Zolnierkiewicz wrote:
> 
> I hope you dont mind Petr.

No problem.
 
But I have one, unrelated... Today I found that VMware does not run
on 2.5.24 with rawdisks for non-root users because of ioctl(hdd, HDIO_GETGEO, ...)
is guarded by "if (!capable(CAP_SYS_ADMIN)) return -EACCES;". And so it
fails although user has read-write access to /dev/hdX.

Is this change really intentional? It is GET, not SET operation, and user has
access to /dev/hdX. If this change is intentional, I'll recommend VMware
to gain priviledges around disk geometry accesses, but I do not think that
user should need SYS_ADMIN for retrieving disk geometry.
					Thanks,
						Petr Vandrovec
						vandrove@vc.cvut.cz

Re: HDIO_GETGEO accessibility (was Re: [PATCH] 2.5.24 IDE 95 (fwd))

[Bartlomiej Zolnierkiewicz]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1712 bytes --]


On Sun, 30 Jun 2002, Petr Vandrovec wrote:

> On Sun, Jun 30, 2002 at 06:52:58PM +0200, Bartlomiej Zolnierkiewicz wrote:
> >
> > I hope you dont mind Petr.
>
> No problem.
>
> But I have one, unrelated... Today I found that VMware does not run
> on 2.5.24 with rawdisks for non-root users because of ioctl(hdd, HDIO_GETGEO, ...)
> is guarded by "if (!capable(CAP_SYS_ADMIN)) return -EACCES;". And so it
> fails although user has read-write access to /dev/hdX.
>
> Is this change really intentional? It is GET, not SET operation, and user has

It changed in IDE-60, comment in ioctl.c says that:

/* Contrary to popular beleve we disallow even the reading of the ioctl
 * values for users which don't have permission too. We do this becouse
 * such information could be used by an attacker to deply a simple-user
 * attack, which triggers bugs present only on a particular
 * configuration.
 */

But I dont think HDIO_GET_* can disclose any meaningful information
to attacker and attacker doesnt have direct access to hardware,
and if he has we have more serious problems to worry about.

[ There is more risk that application programmers will screw
  privilidged access, then attacker will get useful info :-) ]

So ata_ioctl() in ioctl.c needs trivial fix, untested one attached :).
It removes checks for CAP_SYS_ADMIN from HDIO_GET_* ioctls and adds
missing one to BLKRRPART ioctl (re-read partition table).

> access to /dev/hdX. If this change is intentional, I'll recommend VMware
> to gain priviledges around disk geometry accesses, but I do not think that
> user should need SYS_ADMIN for retrieving disk geometry.
> 					Thanks,
> 						Petr Vandrovec
> 						vandrove@vc.cvut.cz

Greets.
--
Bartlomiej

[-- Attachment #2: Type: TEXT/PLAIN, Size: 2290 bytes --]

--- linux-2.5.24/drivers/ide/ioctl.c	Wed Jun 26 00:02:53 2002
+++ linux/drivers/ide/ioctl.c	Sun Jun 30 22:16:50 2002
@@ -122,9 +122,6 @@
 		case HDIO_GET_32BIT: {
 			unsigned long val = drive->channel->io_32bit;
 
-			if (!capable(CAP_SYS_ADMIN))
-				return -EACCES;
-
 			if (put_user(val, (unsigned long *) arg))
 				return -EFAULT;
 			return 0;
@@ -172,9 +169,6 @@
 		case HDIO_GET_UNMASKINTR: {
 			unsigned long val = drive->channel->unmask;
 
-			if (!capable(CAP_SYS_ADMIN))
-				return -EACCES;
-
 			if (put_user(val, (unsigned long *) arg))
 				return -EFAULT;
 
@@ -202,9 +196,6 @@
 		case HDIO_GET_DMA: {
 			unsigned long val = drive->using_dma;
 
-			if (!capable(CAP_SYS_ADMIN))
-				return -EACCES;
-
 			if (put_user(val, (unsigned long *) arg))
 				return -EFAULT;
 
@@ -236,9 +227,6 @@
 			struct hd_geometry *loc = (struct hd_geometry *) arg;
 			unsigned short bios_cyl = drive->bios_cyl; /* truncate */
 
-			if (!capable(CAP_SYS_ADMIN))
-				return -EACCES;
-
 			if (!loc || (drive->type != ATA_DISK && drive->type != ATA_FLOPPY))
 				return -EINVAL;
 
@@ -261,9 +249,6 @@
 		case HDIO_GETGEO_BIG_RAW: {
 			struct hd_big_geometry *loc = (struct hd_big_geometry *) arg;
 
-			if (!capable(CAP_SYS_ADMIN))
-				return -EACCES;
-
 			if (!loc || (drive->type != ATA_DISK && drive->type != ATA_FLOPPY))
 				return -EINVAL;
 
@@ -284,8 +269,6 @@
 		}
 
 		case HDIO_GET_IDENTITY:
-			if (!capable(CAP_SYS_ADMIN))
-				return -EACCES;
 
 			if (minor(inode->i_rdev) & PARTN_MASK)
 				return -EINVAL;
@@ -299,8 +282,6 @@
 			return 0;
 
 		case HDIO_GET_NICE:
-			if (!capable(CAP_SYS_ADMIN))
-				return -EACCES;
 
 			return put_user(drive->dsc_overlap << IDE_NICE_DSC_OVERLAP |
 					drive->atapi_overlap << IDE_NICE_ATAPI_OVERLAP,
@@ -323,8 +304,6 @@
 			return 0;
 
 		case HDIO_GET_BUSSTATE:
-			if (!capable(CAP_SYS_ADMIN))
-				return -EACCES;
 
 			if (put_user(drive->channel->bus_state, (long *)arg))
 				return -EFAULT;
@@ -362,6 +341,9 @@
 			return block_ioctl(inode->i_bdev, cmd, arg);
 
 		case BLKRRPART: /* Re-read partition tables */
+			if (!capable(CAP_SYS_ADMIN))
+				return -EACCES;
+
 			return ata_revalidate(inode->i_rdev);
 
 		case BLKGETSIZE: