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: <20101002074944.GB14377@esdhcp036161.research.nokia.com>
Date:	Sat, 2 Oct 2010 10:49:44 +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 09:21:36PM +0200, ext Guzman Lugo, Fernando wrote:
>  
> > 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.
> > 
> 
> I was taking about vitual memory waste (maybe not so important).
> Is ok for me then keep the gap. Do other changes look good to
> You?

Do you mean in this patch?
All changes make sense only if you're removing the gap, except for the
fix below.

[snip]

> > > > >  
> > > > >  		prev_end = tmp->da_end;
> > > > >  	}
> > > > >  
> > > > > -	if ((start > prev_end) && (ULONG_MAX - start >= bytes))
> > > > > +	if ((start >= prev_end) && (ULONG_MAX - start + 
> > 1 >= bytes))

This fix is partially valid. The correct change must be only:
-	if ((start > prev_end) && (ULONG_MAX - start >= bytes))
+	if ((start > prev_end) && (ULONG_MAX - start + 1 >= bytes))

Otherwise you wouldn't guarantee the gap for fixed da.

Br,

David
--
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/