RFC: banning device driver reserved resources from /dev/mem

RFC: banning device driver reserved resources from /dev/mem

[Arjan van de Ven]

From: Arjan van de Ven <arjan@linux.intel.com>
Date: Sun, 5 Oct 2008 18:00:15 -0700
Subject: [PATCH] resource: don't allow /dev/mem access reserved resources

Device drivers that use pci_request_regions() (and similar APIs) have a
reasonable expectation that they are the only ones accessing their device.
As part of the e1000e hunt, we were afraid that some userland (X or some
bootsplash stuff) was mapping the MMIO region, that the driver thought it
had exclusively, via /dev/mem.

This patch adds, to the existing config option to restrict /dev/mem, the
reserved regions to the "banned from /dev/mem use" list, so now
both kernel memory and device-exclusive MMIO regions are banned.

The introduced iomem_is_reserved() function is also planned to be used
for other patches in 2.6.28 (pci_ioremap) so is exported here as part
of being introduced.

Signed-of-by: Arjan van de Ven <arjan@linux.intel.com>
---
 arch/x86/mm/init_32.c  |    2 ++
 arch/x86/mm/init_64.c  |    2 ++
 include/linux/ioport.h |    1 +
 kernel/resource.c      |   32 ++++++++++++++++++++++++++++++++
 4 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index 63b71d3..c98f5e8 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -329,6 +329,8 @@ int devmem_is_allowed(unsigned long pagenr)
 {
 	if (pagenr <= 256)
 		return 1;
+	if (iomem_is_reserved(pagenr << PAGE_SHIFT))
+		return 0;
 	if (!page_is_ram(pagenr))
 		return 1;
 	return 0;
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 747f6c9..0c7037c 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -892,6 +892,8 @@ int devmem_is_allowed(unsigned long pagenr)
 {
 	if (pagenr <= 256)
 		return 1;
+	if (iomem_is_reserved(pagenr << PAGE_SHIFT))
+		return 0;
 	if (!page_is_ram(pagenr))
 		return 1;
 	return 0;
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index e38b6aa..eee0f61 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -170,6 +170,7 @@ extern struct resource * __devm_request_region(struct device *dev,
 extern void __devm_release_region(struct device *dev, struct resource *parent,
 				  resource_size_t start, resource_size_t n);
 extern int iomem_map_sanity_check(resource_size_t addr, unsigned long size);
+extern int iomem_is_reserved(u64 addr);
 
 #endif /* __ASSEMBLY__ */
 #endif	/* _LINUX_IOPORT_H */
diff --git a/kernel/resource.c b/kernel/resource.c
index 7797dae..e217083 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -864,3 +864,35 @@ int iomem_map_sanity_check(resource_size_t addr, unsigned long size)
 
 	return err;
 }
+
+/*
+ * check if an address is reserved in the iomem resource tree
+ * returns 1 if reserved, 0 if not reserved.
+ */
+int iomem_is_reserved(u64 addr)
+{
+	struct resource *p = &iomem_resource;
+	int err = 0;
+	loff_t l;
+	int size= PAGE_SIZE;
+
+	read_lock(&resource_lock);
+	for (p = p->child; p ; p = r_next(NULL, p, &l)) {
+		/*
+		 * We can probably skip the resources without
+		 * IORESOURCE_IO attribute?
+		 */
+		if (p->start >= addr + size)
+			continue;
+		if (p->end < addr)
+			continue;
+		if (p->flags & IORESOURCE_BUSY) {
+			err = 1;
+			break;
+		}
+	}
+	read_unlock(&resource_lock);
+
+	return err;
+}
+EXPORT_SYMBOL(iomem_is_reserved);
-- 
1.5.5.1


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: RFC: banning device driver reserved resources from /dev/mem

[Ingo Molnar]

* Arjan van de Ven <arjan@infradead.org> wrote:

> From: Arjan van de Ven <arjan@linux.intel.com>
> Date: Sun, 5 Oct 2008 18:00:15 -0700
> Subject: [PATCH] resource: don't allow /dev/mem access reserved resources
> 
> Device drivers that use pci_request_regions() (and similar APIs) have a
> reasonable expectation that they are the only ones accessing their device.
> As part of the e1000e hunt, we were afraid that some userland (X or some
> bootsplash stuff) was mapping the MMIO region, that the driver thought it
> had exclusively, via /dev/mem.
> 
> This patch adds, to the existing config option to restrict /dev/mem, the
> reserved regions to the "banned from /dev/mem use" list, so now
> both kernel memory and device-exclusive MMIO regions are banned.
> 
> The introduced iomem_is_reserved() function is also planned to be used
> for other patches in 2.6.28 (pci_ioremap) so is exported here as part
> of being introduced.
> 
> Signed-of-by: Arjan van de Ven <arjan@linux.intel.com>
> ---
>  arch/x86/mm/init_32.c  |    2 ++
>  arch/x86/mm/init_64.c  |    2 ++
>  include/linux/ioport.h |    1 +
>  kernel/resource.c      |   32 ++++++++++++++++++++++++++++++++
>  4 files changed, 37 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
> index 63b71d3..c98f5e8 100644
> --- a/arch/x86/mm/init_32.c
> +++ b/arch/x86/mm/init_32.c
> @@ -329,6 +329,8 @@ int devmem_is_allowed(unsigned long pagenr)
>  {
>  	if (pagenr <= 256)
>  		return 1;
> +	if (iomem_is_reserved(pagenr << PAGE_SHIFT))
> +		return 0;

looks good and useful to me. One small request: could you please stick a 
big fat WARN_ONCE() into this codepath as well?

and it's properly dependent on CONFIG_STRICT_DEVMEM=y [which is 
default-off], so it's not a legacy ABI breaker either.

another small detail:

> +int iomem_is_reserved(u64 addr)
> +{
> +     struct resource *p = &iomem_resource;
> +     int err = 0;
> +     loff_t l;
> +     int size= PAGE_SIZE;
> +
> +     read_lock(&resource_lock);
> +     for (p = p->child; p ; p = r_next(NULL, p, &l)) {
> +             /*
> +              * We can probably skip the resources without
> +              * IORESOURCE_IO attribute?
> +              */
> +             if (p->start >= addr + size)
> +                     continue;

do we want to skip all resources that are not IORESOURCE_MEM? Same holds 
for iomem_map_sanity_check(), introduced in tip/core/resources:

 379daf6: IO resources, x86: ioremap sanity check to catch mapping requests exceeding the BAR sizes

on which you seem to have based iomem_is_reserved().

Perhaps even base both iomem_map_sanity_check() and iomem_is_reserved() 
on a single helper function, and unify the iterator and the overlap 
check? The two have a very similar purpose.

	Ingo

Re: RFC: banning device driver reserved resources from /dev/mem

[Arjan van de Ven]

On Mon, 6 Oct 2008 07:23:37 +0200
Ingo Molnar <mingo@elte.hu> wrote:
> 
> > +int iomem_is_reserved(u64 addr)
> > +{
> > +     struct resource *p = &iomem_resource;

...

> > +              */
> > +             if (p->start >= addr + size)
> > +                     continue;
> 
> do we want to skip all resources that are not IORESOURCE_MEM? Same
> holds for iomem_map_sanity_check(), introduced in tip/core/resources:

if we have non-IORESOURCE_MEM resources in the iomem_resource tree....
then we have bigger problems.

-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: RFC: banning device driver reserved resources from /dev/mem

[Alan Cox]

> This patch adds, to the existing config option to restrict /dev/mem, the
> reserved regions to the "banned from /dev/mem use" list, so now
> both kernel memory and device-exclusive MMIO regions are banned.

This breaks a whole class of diagnostic and debug tools which quite
intentionally dump the MMIO register space of devices that are live.

Plus it doesn't actually work - if I have /dev/mem open and mmapped you
don't pull the pages from under me....

The problem with the way this is going is the more you take from /dev/mem
the sooner someone is forced to add /dev/mem-proper which undoes it all
again so they can get work done.

It's certainly a useful debug aid to know about such things but I don't
believe it's something you should ban.

Alan

Re: RFC: banning device driver reserved resources from /dev/mem

[Maxim Levitsky]

Alan Cox wrote:
>> This patch adds, to the existing config option to restrict /dev/mem, the
>> reserved regions to the "banned from /dev/mem use" list, so now
>> both kernel memory and device-exclusive MMIO regions are banned.
> 
> This breaks a whole class of diagnostic and debug tools which quite
> intentionally dump the MMIO register space of devices that are live.
> 
> Plus it doesn't actually work - if I have /dev/mem open and mmapped you
> don't pull the pages from under me....
> 
> The problem with the way this is going is the more you take from /dev/mem
> the sooner someone is forced to add /dev/mem-proper which undoes it all
> again so they can get work done.
> 
> It's certainly a useful debug aid to know about such things but I don't
> believe it's something you should ban.

Exactly,

I think the proper solution is to make /dev/mem pure diagnostic tool.
Drivers should use /sys mappings of pci devices to access their iomem
, or even better they should not touch pci mappings
directly at all, but talk to kernel, and if xorg needs access 
to video memory it should talk to drm and get its mapping from there.


Best regards,
	Maxim Levitsky

Re: RFC: banning device driver reserved resources from /dev/mem

[Alan Cox]

> I think the proper solution is to make /dev/mem pure diagnostic tool.
> Drivers should use /sys mappings of pci devices to access their iomem

Only a tiny fraction of devices in the world are PCI, and those platforms
where you most what /dev/mem type debugging tools are often those without
PCI

> , or even better they should not touch pci mappings
> directly at all, but talk to kernel, and if xorg needs access 
> to video memory it should talk to drm and get its mapping from there.

Most video cards don't use DRM, but the kernel does have the same video
memory mapped for text consoles.

Alan

Re: RFC: banning device driver reserved resources from /dev/mem

[Arjan van de Ven]

On Mon, 6 Oct 2008 14:48:35 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > I think the proper solution is to make /dev/mem pure diagnostic
> > tool. Drivers should use /sys mappings of pci devices to access
> > their iomem
> 
> Only a tiny fraction of devices in the world are PCI, and those
> platforms where you most what /dev/mem type debugging tools are often
> those without PCI

if you looked at the patch it's x86 only, and only active if you set
CONFIG_STRICT_DEVMEM.
(if you don't select CONFIG_STRICT_DEVMEM, which is off by default, you
get unlimited access as usualy)



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: RFC: banning device driver reserved resources from /dev/mem

[Alan Cox]

> > Only a tiny fraction of devices in the world are PCI, and those
> > platforms where you most what /dev/mem type debugging tools are often
> > those without PCI
> 
> if you looked at the patch it's x86 only, and only active if you set
> CONFIG_STRICT_DEVMEM.
> (if you don't select CONFIG_STRICT_DEVMEM, which is off by default, you
> get unlimited access as usualy)

Lots of non PCI devices on x86 systems as well.

And the video problem is a big one for the x86 case as the VGA ISA window
is used by both the kernel and X without a helper driver.

Alan

Re: RFC: banning device driver reserved resources from /dev/mem

[Arjan van de Ven]

On Mon, 6 Oct 2008 15:01:22 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > > Only a tiny fraction of devices in the world are PCI, and those
> > > platforms where you most what /dev/mem type debugging tools are
> > > often those without PCI
> > 
> > if you looked at the patch it's x86 only, and only active if you set
> > CONFIG_STRICT_DEVMEM.
> > (if you don't select CONFIG_STRICT_DEVMEM, which is off by default,
> > you get unlimited access as usualy)
> 
> Lots of non PCI devices on x86 systems as well.
> 
> And the video problem is a big one for the x86 case as the VGA ISA
> window is used by both the kernel and X without a helper driver.

no argument, except that the kernel doesn't do request_region() on it
to expect exclusivity (so the patch doesn't do anything on this region)

Now having said that, if the DRM layer does request_region on the MMIO
bars, we might need a flag that explicitly says "this is intended for
sharing with userspace" for this known case; not too hard, I'll check
with Dave Airlie.


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: RFC: banning device driver reserved resources from /dev/mem

[Alan Cox]

> no argument, except that the kernel doesn't do request_region() on it
> to expect exclusivity (so the patch doesn't do anything on this region)

Its on the TODO list for the vt fixes

> Now having said that, if the DRM layer does request_region on the MMIO
> bars, we might need a flag that explicitly says "this is intended for
> sharing with userspace" for this known case; not too hard, I'll check
> with Dave Airlie.

DRM isn't the problem. In the DRM case the DRM can provide the mappings
and manage them. In the non DRM case its messier but for the moment
probably happens by luck to be ok

I still think your restrictive /dev/mem model is wrong. I think it comes
about because your /dev/mem restrictions are for multiple conflicting
purposes.

The root of that is the 'vaguely annoy root kit writers for 5 minutes'
reasoning which erroneously leads to trying to a compile time option,
combined some would argue with a 'screw people hacking hardware in
userspace who should provide drivers' view.

Now the root kit writer one has minimal credence now as it seems the root
kit writers already adapted. The hardware hacking people have uio
frameworks and should yes be encouraged to use proper drivers.

So there isn't a real reason to make it compile time. Far saner would be
for the /dev/mem restrictions to be flags in sysfs. Even if you stick to
the view about making life harder for rootkit authors then it should be
sysfs restrictions with an irrevocability bit (eg 0 = off 1 = on , 2 = on
(forever))

A flag per feature of this kind then lets you set the different policies
including the highly useful '/dev/mem, I don't think so' policy flag for
server boxes.

Re: RFC: banning device driver reserved resources from /dev/mem

[Arjan van de Ven]

On Mon, 6 Oct 2008 15:14:11 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > no argument, except that the kernel doesn't do request_region() on
> > it to expect exclusivity (so the patch doesn't do anything on this
> > region)
> 
> Its on the TODO list for the vt fixes
> 
> > Now having said that, if the DRM layer does request_region on the
> > MMIO bars, we might need a flag that explicitly says "this is
> > intended for sharing with userspace" for this known case; not too
> > hard, I'll check with Dave Airlie.
> 
> DRM isn't the problem. In the DRM case the DRM can provide the
> mappings and manage them. In the non DRM case its messier but for the
> moment probably happens by luck to be ok

I'll add a "share resource with userland" option to allow us to make
this desire explicit for the cases we want this (and can tolerate
concurrent accesses)

> 
> I still think your restrictive /dev/mem model is wrong. I think it
> comes about because your /dev/mem restrictions are for multiple
> conflicting purposes.

for me it is a goal to have /dev/mem do as little as possible while
allowing the "normal" uses. This is to help SELinux to have sane policy
rather than "X still has perms to own the whole box" etc.

> 
> The root of that is the 'vaguely annoy root kit writers for 5 minutes'
> reasoning which erroneously leads to trying to a compile time option,
> combined some would argue with a 'screw people hacking hardware in
> userspace who should provide drivers' view.

this patch absolutely has nothing to do with rootkits; really.
it came out of chasing e1000e with the "eh who maps our e1000e bar from
userspace" scare. Followed by thinking "if the driver requests
exclusivity the kernel should try to grant that".

-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: RFC: banning device driver reserved resources from /dev/mem

[Alan Cox]

> I'll add a "share resource with userland" option to allow us to make
> this desire explicit for the cases we want this (and can tolerate
> concurrent accesses)

For debug tools that would be almost all drivers, and for distribution
use that needs to be a runtime selection.

For the non share-resource-with case what occurs if /dev/mem is active
when the driver is loaded ?

> for me it is a goal to have /dev/mem do as little as possible while
> allowing the "normal" uses. This is to help SELinux to have sane policy
> rather than "X still has perms to own the whole box" etc.

Then it should be runtime configurable

> it came out of chasing e1000e with the "eh who maps our e1000e bar from
> userspace" scare. Followed by thinking "if the driver requests
> exclusivity the kernel should try to grant that".

Only if you can then configure that *policy* decision at runtime in user
space. Otherwise you create shackles that simply harm debug work and make
it harder for distributions to ship the feature enabled by default (which
is the ideal case).

Alan

Re: RFC: banning device driver reserved resources from /dev/mem

[Arjan van de Ven]

> 
> For debug tools that would be almost all drivers, and for distribution
> use that needs to be a runtime selection.

is boottime (read: kernel parameter) ok ?
it would totally make sense to have a kernel boot parameter that
disables all /dev/mem sanity checks.

> 
> For the non share-resource-with case what occurs if /dev/mem is active
> when the driver is loaded ?

that's a hard case but probably not the most interesting case; drivers
get loaded early thankfully.
(yes revoke is hard ;( )



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: RFC: banning device driver reserved resources from /dev/mem

[Alan Cox]

On Mon, 6 Oct 2008 07:57:38 -0700
Arjan van de Ven <arjan@infradead.org> wrote:

> > 
> > For debug tools that would be almost all drivers, and for distribution
> > use that needs to be a runtime selection.
> 
> is boottime (read: kernel parameter) ok ?
> it would totally make sense to have a kernel boot parameter that
> disables all /dev/mem sanity checks.

A boot parameter probably catches most cases yes.

> that's a hard case but probably not the most interesting case; drivers
> get loaded early thankfully.
> (yes revoke is hard ;( )

Yeah .. I was hoping you'd write it ;)