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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20081006052337.GE2186@elte.hu>
Date:	Mon, 6 Oct 2008 07:23:37 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Arjan van de Ven <arjan@...radead.org>
Cc:	linux-kernel@...r.kernel.org,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>, Yinghai Lu <yinghai@...nel.org>
Subject: Re: RFC: banning device driver reserved resources from /dev/mem


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

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