linux-kernel - Re: [PATCH STABLE 4.9 1/1] mm, gup: add missing refcount overflow checks on x86 and s390

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]

Date:   Tue, 3 Dec 2019 13:46:59 +0100
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Ben Hutchings <ben.hutchings@...ethink.co.uk>,
        stable@...r.kernel.org
Cc:     Matthew Wilcox <willy@...radead.org>,
        Ajay Kaher <akaher@...are.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH STABLE 4.9 1/1] mm, gup: add missing refcount overflow
 checks on x86 and s390

On 12/3/19 1:22 PM, Ben Hutchings wrote:
>> +		    || !page_cache_get_speculative(head)))
>>  			return 0;
>>  		if (unlikely(pte_val(pte) != pte_val(*ptep))) {
>>  			put_page(head);
> [...]
>> --- a/arch/x86/mm/gup.c
>> +++ b/arch/x86/mm/gup.c
>> @@ -202,10 +202,12 @@ static int __gup_device_huge_pmd(pmd_t pmd, unsigned long addr,
>>  			undo_dev_pagemap(nr, nr_start, pages);
>>  			return 0;
>>  		}
>> +		if (unlikely(!try_get_page(page))) {
>> +			put_dev_pagemap(pgmap);
>> +			return 0;
>> +		}
>>  		SetPageReferenced(page);
>>  		pages[*nr] = page;
>> -		get_page(page);
>> -		put_dev_pagemap(pgmap);
> 
> This leaks a pgmap reference on success!

Good catch, deleted one line too many!

>>  		(*nr)++;
>>  		pfn++;
>>  	} while (addr += PAGE_SIZE, addr != end);
>> @@ -230,6 +232,8 @@ static noinline int gup_huge_pmd(pmd_t pmd, unsigned long addr,
>>  
>>  	refs = 0;
>>  	head = pmd_page(pmd);
>> +	if (WARN_ON_ONCE(page_ref_count(head) <= 0))
> 
> Why <= 0, given we use < 0 elsewhere?

The code uses get_head_page_multiple() which boils down to atomic_add
and not add_unless_zero(), so it assumes a pre-existing pin that must
not go away or it's a bug (one that I've been hunting recently in this
area). The check makes it explicit.

> 
>> +		return 0;
>>  	page = head + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
>>  	do {
>>  		VM_BUG_ON_PAGE(compound_head(page) != head, page);
>> @@ -289,6 +293,8 @@ static noinline int gup_huge_pud(pud_t pud, unsigned long addr,
>>  
>>  	refs = 0;
>>  	head = pud_page(pud);
>> +	if (WARN_ON_ONCE(page_ref_count(head) <= 0))
> 
> Same question here.

Same as above.

> Ben.
> 
>> +		return 0;
>>  	page = head + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
>>  	do {
>>  		VM_BUG_ON_PAGE(compound_head(page) != head, page);