Subject: Re: [PATCH] 2.5.24 IDE 95 (fwd)


I hope you dont mind Petr.

---------- Forwarded message ----------
Date: Sun, 30 Jun 2002 18:20:03 +0200 (MET DST)
From: Bartlomiej Zolnierkiewicz <[email protected]>
To: Petr Vandrovec <[email protected]>
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


Attachments:
ide-intr.txt (2.38 kB)

2002-06-30 19:50:38

by Petr Vandrovec

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

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
[email protected]

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


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
> [email protected]

Greets.
--
Bartlomiej


Attachments:
ioctls-fix.diff (2.24 kB)