linux-kernel - Re: [PATCH] x86: Fix CPA memtype reserving in the set_pages

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 for Android: free password hash cracker in your pocket

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

Date:	Mon, 03 Aug 2009 10:39:43 +0200
From:	Thomas Hellstrom <thellstrom@...are.com>
To:	Dave Airlie <airlied@...ux.ie>
CC:	Ingo Molnar <mingo@...e.hu>,
	Arjan van de Ven <arjan@...radead.org>,
	"suresh.b.siddha@...el.com" <suresh.b.siddha@...el.com>,
	"hpa@...or.com" <hpa@...or.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"venkatesh.pallipadi@...el.com" <venkatesh.pallipadi@...el.com>,
	"tglx@...utronix.de" <tglx@...utronix.de>,
	"dri-devel@...ts.sourceforge.net" <dri-devel@...ts.sourceforge.net>
Subject: Re: [PATCH] x86: Fix CPA memtype reserving in the set_pages_array
 cases

Dave Airlie wrote:
>> hm, i'm missing a description about how this bug was 
>> triggered. How did you end up getting highmem pages to a cpa 
>> call?
>>     
>
> GEM and TTM both allocate page arrays and just pass them to cpa,
> we don't know what type of pages the allocator gives us back and we really 
> shouldn't have to, so having cpa ignore highmem pages is certainly the 
> right option.
>
> GEM just uses shmem code to alloc the pages and TTM has its own allocator.
>
>   
Yes, Dave is right.

Although I'm not 100% sure the TTM code I was using that triggered this 
has made it into 2.6.31.
Old AGP uses (GFP_KERNEL | GFP_DMA32 | __GFP_ZERO), which (correct me if 
I'm wrong) never hands back highmem pages. This means that Intel's GEM 
is the only likely user for 2.6.31.

/Thomas

> Dave.
>
>  > 
>   
>>>  	else
>>>  		address = *cpa->vaddr;
>>> @@ -696,9 +699,12 @@ static int cpa_process_alias(struct cpa_data *cpa)
>>>  	 * No need to redo, when the primary call touched the direct
>>>  	 * mapping already:
>>>  	 */
>>> -	if (cpa->flags & CPA_PAGES_ARRAY)
>>> -		vaddr = (unsigned long)page_address(cpa->pages[cpa->curpage]);
>>> -	else if (cpa->flags & CPA_ARRAY)
>>> +	if (cpa->flags & CPA_PAGES_ARRAY) {
>>> +		struct page *page = cpa->pages[cpa->curpage];
>>> +		if (unlikely(PageHighMem(page)))
>>> +			return 0;
>>> +		vaddr = (unsigned long)page_address(page);
>>> +	} else if (cpa->flags & CPA_ARRAY)
>>>  		vaddr = cpa->vaddr[cpa->curpage];
>>>  	else
>>>  		vaddr = *cpa->vaddr;
>>> @@ -1118,7 +1124,9 @@ int set_pages_array_uc(struct page **pages, int addrinarray)
>>>  	int free_idx;
>>>  
>>>  	for (i = 0; i < addrinarray; i++) {
>>> -		start = (unsigned long)page_address(pages[i]);
>>> +		if (PageHighMem(pages[i]))
>>> +			continue;
>>> +		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
>>>  		end = start + PAGE_SIZE;
>>>  		if (reserve_memtype(start, end, _PAGE_CACHE_UC_MINUS, NULL))
>>>  			goto err_out;
>>>       
>> ok, that's a bug introduced in .29 but which was latent until now: 
>> drivers/char/agp/generic.c now uses it plus (indirectly) a number of 
>> AGP drivers, since:
>>
>>  commit 07613ba2f464f59949266f4337b75b91eb610795
>>  Author: Dave Airlie <airlied@...hat.com>
>>  Date:   Fri Jun 12 14:11:41 2009 +1000
>>
>>      agp: switch AGP to use page array instead of unsigned long array
>>
>> I dont see how it can end up with highmem pages though. All 
>> the graphics apperture allocations happen to lowmem AFAICS. 
>> Did GEM add the possibility for user pages (highmem amongst 
>> them) ending up in that pool? Which code does that?
>>
>>     
>>> @@ -1131,7 +1139,9 @@ int set_pages_array_uc(struct page **pages, int addrinarray)
>>>  err_out:
>>>  	free_idx = i;
>>>  	for (i = 0; i < free_idx; i++) {
>>> -		start = (unsigned long)page_address(pages[i]);
>>> +		if (PageHighMem(pages[i]))
>>> +			continue;
>>> +		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
>>>  		end = start + PAGE_SIZE;
>>>  		free_memtype(start, end);
>>>  	}
>>> @@ -1160,7 +1170,9 @@ int set_pages_array_wb(struct page **pages, int addrinarray)
>>>  		return retval;
>>>  
>>>  	for (i = 0; i < addrinarray; i++) {
>>> -		start = (unsigned long)page_address(pages[i]);
>>> +		if (PageHighMem(pages[i]))
>>> +			continue;
>>> +		start = page_to_pfn(pages[i]) << PAGE_SHIFT;
>>>  		end = start + PAGE_SIZE;
>>>  		free_memtype(start, end);
>>>       
>> In any case it's a must-have fix for .31. Possibly even a backport 
>> tag is needed, in case a distro does a .30 kernel with more recent 
>> graphics bits.
>>
>> 	Ingo
>>
>> ------------------------------------------------------------------------------
>> Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day 
>> trial. Simplify your report design, integration and deployment - and focus on 
>> what you do best, core application coding. Discover what's new with 
>> Crystal Reports now.  http://p.sf.net/sfu/bobj-july
>> --
>> _______________________________________________
>> Dri-devel mailing list
>> Dri-devel@...ts.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/dri-devel
>>
>>
>>     

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