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

[Ingo Molnar]

* Arjan van de Ven <arjan@xxxxxxxxxxxxx> wrote:

> From: Arjan van de Ven <arjan@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxx>
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/