linux-kernel - Re: [PATCH 2/4] iovmm: fix roundup for next area and end check for the last area

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: <20101001175327.GA13657@esdhcp036161.research.nokia.com>
Date:	Fri, 1 Oct 2010 20:53:27 +0300
From:	David Cohen <david.cohen@...ia.com>
To:	"ext Guzman Lugo, Fernando" <fernando.lugo@...com>
Cc:	"Doyu Hiroshi (Nokia-MS/Espoo)" <hiroshi.doyu@...ia.com>,
	"Contreras Felipe (Nokia-MS/Helsinki)" <felipe.contreras@...ia.com>,
	"Palande Ameya (Nokia-MS/Helsinki)" <ameya.palande@...ia.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>
Subject: Re: [PATCH 2/4] iovmm: fix roundup for next area and end check for
 the last area

On Fri, Oct 01, 2010 at 06:10:30PM +0200, ext Guzman Lugo, Fernando wrote:
>  

[snip]

> > >  arch/arm/plat-omap/iovmm.c |    6 +++---
> > >  1 files changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/arch/arm/plat-omap/iovmm.c 
> > b/arch/arm/plat-omap/iovmm.c 
> > > index 24ca9c4..fc6b109 100644
> > > --- a/arch/arm/plat-omap/iovmm.c
> > > +++ b/arch/arm/plat-omap/iovmm.c
> > > @@ -289,19 +289,19 @@ static struct iovm_struct 
> > *alloc_iovm_area(struct iommu *obj, u32 da,
> > >  	prev_end = 0;
> > >  	list_for_each_entry(tmp, &obj->mmap, list) {
> > >  
> > > -		if (prev_end >= start)
> > > +		if (prev_end > start)
> > >  			break;
> > >  
> > >  		if (start + bytes <= tmp->da_start)
> > >  			goto found;
> > >  
> > >  		if (flags & IOVMF_DA_ANON)
> > > -			start = roundup(tmp->da_end + 1, alignement);
> > > +			start = roundup(tmp->da_end, alignement);
> > 
> > There's a lack of comment here, but the purpose of 
> > tmp->da_end + 1 is to create a gap between iovm areas to 
> > force to trigger iommu faults when some access exceeds a 
> > valid area. Without this gap, such situation may produce data 
> > corruption which is much more difficult to track.
> 
> That only works when you are accessing sequencially beyond the
> End of the vm_area. However if you are accessing a random address
> Which is in the mmu tables you still can corrupt memory which does
> Not belong to you. That looks not very effective then why waste
> Memory?

The main intention is to detect sequential access beyond the end of the
vm area and it is effective for that purpose.
i.e., OMAP3 ISP has a hw issue which makes its H3A submodule, responsible
to produce statistics data for the captured image, to write more data than
it should. The workaround described in the errata wasn't enough to avoid
error conditions, so a different approach was implemented. This gap
did help me to make sure the new workaround is valid and no data
corruption was occurring anymore.
Anyway, I can't see why memory is being wasted.

> 
> Maybe other mechanism should be implemente like in the process
> Switching when if the process has DMM virtual memory area and if
> So enablig only that area (all other process areas will be
> Dissabled and it would get a mmufault in case of access). However
> That increase the time of switching between process.

Sure. We can have other mechanisms. But in the above scenario, H3A
submodule has sequential access and can corrupt its own data. It has
more than one buffer and they're likely to be mapped to sequential
memory areas without the mechanism you're about to remove.

If you're able to implement a better mechanism, please remove this one
just when a new is already there. :)

Regards,

David

> 
> Regards,
> Fernando.
> 
> > 
> > Br,
> > 
> > David
> > 
> > >  
> > >  		prev_end = tmp->da_end;
> > >  	}
> > >  
> > > -	if ((start > prev_end) && (ULONG_MAX - start >= bytes))
> > > +	if ((start >= prev_end) && (ULONG_MAX - start + 1 >= bytes))
> > >  		goto found;
> > >  
> > >  	dev_dbg(obj->dev, "%s: no space to fit %08x(%x) flags: %08x\n",
> > > --
> > > 1.6.3.3
> > > 
> > > --
> > > 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/
> > 
--
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/