[PATCH V2] arm64/mm: Fix pgtable page offset address in [pud|pmd]_free_[pmd|pte]_page

[PATCH V2] arm64/mm: Fix pgtable page offset address in [pud|pmd]_free_[pmd|pte]_page

[Anshuman Khandual]

The pgtable page address can be fetched with [pmd|pte]_offset_[kernel] if
the input address is either PMD_SIZE or PTE_SIZE aligned. Though incoming
input addresses tend to be aligned to the required size (PMD_SIZE|PTE_SIZE)
which is the reason why there were no reported problems earlier. But it is
not correct. However 0UL as input address will guarantee that the fetched
pgtable page address is always correct without any possible skid. While at
this just warn once when the addresses are not aligned.

Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
Changes in V2:

- Replaced WARN_ON_ONCE() with VM_WARN_ONCE() as per Catalin

 arch/arm64/mm/mmu.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index e97f018ff740..9dc1c7e90ef4 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -1005,7 +1005,9 @@ int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
 		return 1;
 	}
 
-	table = pte_offset_kernel(pmdp, addr);
+	VM_WARN_ONCE(!IS_ALIGNED(addr, PMD_SIZE),
+		"%s: unaligned address: 0x%016llx\n", __func__, addr);
+	table = pte_offset_kernel(pmdp, 0UL);
 	pmd_clear(pmdp);
 	__flush_tlb_kernel_pgtable(addr);
 	pte_free_kernel(NULL, table);
@@ -1026,8 +1028,10 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
 		return 1;
 	}
 
-	table = pmd_offset(pudp, addr);
-	pmdp = table;
+	VM_WARN_ONCE(!IS_ALIGNED(addr, PUD_SIZE),
+		"%s: unaligned address 0x%016llx\n", __func__, addr);
+	table = pmd_offset(pudp, 0UL);
+	pmdp = pmd_offset(pudp, addr);
 	next = addr;
 	end = addr + PUD_SIZE;
 	do {
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2] arm64/mm: Fix pgtable page offset address in [pud|pmd]_free_[pmd|pte]_page

[Catalin Marinas]

On Tue, Apr 30, 2019 at 09:13:59AM +0530, Anshuman Khandual wrote:
> @@ -1026,8 +1028,10 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
>  		return 1;
>  	}
>  
> -	table = pmd_offset(pudp, addr);
> -	pmdp = table;
> +	VM_WARN_ONCE(!IS_ALIGNED(addr, PUD_SIZE),
> +		"%s: unaligned address 0x%016llx\n", __func__, addr);
> +	table = pmd_offset(pudp, 0UL);
> +	pmdp = pmd_offset(pudp, addr);

Why does pmdp need to use addr? We are freeing the whole pmd page, so I
don't think pmdp should be different from table here.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2] arm64/mm: Fix pgtable page offset address in [pud|pmd]_free_[pmd|pte]_page

[Anshuman Khandual]

On 04/30/2019 09:47 PM, Catalin Marinas wrote:
> On Tue, Apr 30, 2019 at 09:13:59AM +0530, Anshuman Khandual wrote:
>> @@ -1026,8 +1028,10 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
>>  		return 1;
>>  	}
>>  
>> -	table = pmd_offset(pudp, addr);
>> -	pmdp = table;
>> +	VM_WARN_ONCE(!IS_ALIGNED(addr, PUD_SIZE),
>> +		"%s: unaligned address 0x%016llx\n", __func__, addr);
>> +	table = pmd_offset(pudp, 0UL);
>> +	pmdp = pmd_offset(pudp, addr);
> 
> Why does pmdp need to use addr? We are freeing the whole pmd page, so I
> don't think pmdp should be different from table here.

pmd_offset(pudp, addr) and pmd_offset(pudp, 0UL) would evaluate the same if
the input addr is PMD_SIZE aligned. The problem just arises when it is not.

The Idea is that the wrong input should be allowed to have adverse affect
all the way without any corrections. Now because intermediate 'next' and
'end' are derived from wrong input 'addr' in the first place, all 'pmdp'
start and intermediate values should just follow without any corrections
starting at pmd_offset(pudp, addr).

The new warning here just informs about the fact that the iteration range is
going to be wrong (as well as everything else probably) because the input
address is not aligned. 'table = pmd_offset(pudp, 0UL)' will prevent it from
hitting unaligned BUG_ON() in pmd_free(). 

Using 'pmdp = pmd_offset(pudp, 0UL)' for iterations will be sort of fixing
or aligning the wrong input 'addr' which we always wanted to avoid.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2] arm64/mm: Fix pgtable page offset address in [pud|pmd]_free_[pmd|pte]_page

[Catalin Marinas]

On Wed, May 01, 2019 at 10:15:49AM +0530, Anshuman Khandual wrote:
> On 04/30/2019 09:47 PM, Catalin Marinas wrote:
> > On Tue, Apr 30, 2019 at 09:13:59AM +0530, Anshuman Khandual wrote:
> >> @@ -1026,8 +1028,10 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
> >>  		return 1;
> >>  	}
> >>  
> >> -	table = pmd_offset(pudp, addr);
> >> -	pmdp = table;
> >> +	VM_WARN_ONCE(!IS_ALIGNED(addr, PUD_SIZE),
> >> +		"%s: unaligned address 0x%016llx\n", __func__, addr);
> >> +	table = pmd_offset(pudp, 0UL);
> >> +	pmdp = pmd_offset(pudp, addr);
> > 
> > Why does pmdp need to use addr? We are freeing the whole pmd page, so I
> > don't think pmdp should be different from table here.
> 
> pmd_offset(pudp, addr) and pmd_offset(pudp, 0UL) would evaluate the same if
> the input addr is PMD_SIZE aligned. The problem just arises when it is not.
> 
> The Idea is that the wrong input should be allowed to have adverse affect
> all the way without any corrections. Now because intermediate 'next' and
> 'end' are derived from wrong input 'addr' in the first place, all 'pmdp'
> start and intermediate values should just follow without any corrections
> starting at pmd_offset(pudp, addr).
> 
> The new warning here just informs about the fact that the iteration range is
> going to be wrong (as well as everything else probably) because the input
> address is not aligned. 'table = pmd_offset(pudp, 0UL)' will prevent it from
> hitting unaligned BUG_ON() in pmd_free(). 
> 
> Using 'pmdp = pmd_offset(pudp, 0UL)' for iterations will be sort of fixing
> or aligning the wrong input 'addr' which we always wanted to avoid.

So you want to hide the BUG_ON in pmd_free() by changing this to a
warning in the caller (pud_free_pmd_page()) and sanitising the value
passed to pmd_free(). I don't see how this is different from just
turning the BUG_ON into a warning in pmd_free() directly (which I don't
think we should, see below).

It looks to me like we should either fix the callers in ioremap.c (e.g.
ioremap_try_huge_pud() returning 0 if addr is not PUD_SIZE aligned, not
just phys_addr) or return 0 in pud_free_pmd_page() with a similar check.
I'd go for changing ioremap.c since x86 doesn't have such check either.

IIUC currently if we pass a PUD_SIZE range to ioremap_page_range() where
phys_addr is PUD_SIZE aligned but the virtual addr is not, we'd end up
randomly freeing the pmd page that potentially still contains valid
mappings. Your patch just hides the problem by turning the BUG_ON into a
warning but doesn't solve it.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2] arm64/mm: Fix pgtable page offset address in [pud|pmd]_free_[pmd|pte]_page

[Anshuman Khandual]

On 05/01/2019 04:44 PM, Catalin Marinas wrote:
> On Wed, May 01, 2019 at 10:15:49AM +0530, Anshuman Khandual wrote:
>> On 04/30/2019 09:47 PM, Catalin Marinas wrote:
>>> On Tue, Apr 30, 2019 at 09:13:59AM +0530, Anshuman Khandual wrote:
>>>> @@ -1026,8 +1028,10 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
>>>>  		return 1;
>>>>  	}
>>>>  
>>>> -	table = pmd_offset(pudp, addr);
>>>> -	pmdp = table;
>>>> +	VM_WARN_ONCE(!IS_ALIGNED(addr, PUD_SIZE),
>>>> +		"%s: unaligned address 0x%016llx\n", __func__, addr);
>>>> +	table = pmd_offset(pudp, 0UL);
>>>> +	pmdp = pmd_offset(pudp, addr);
>>>
>>> Why does pmdp need to use addr? We are freeing the whole pmd page, so I
>>> don't think pmdp should be different from table here.
>>
>> pmd_offset(pudp, addr) and pmd_offset(pudp, 0UL) would evaluate the same if
>> the input addr is PMD_SIZE aligned. The problem just arises when it is not.
>>
>> The Idea is that the wrong input should be allowed to have adverse affect
>> all the way without any corrections. Now because intermediate 'next' and
>> 'end' are derived from wrong input 'addr' in the first place, all 'pmdp'
>> start and intermediate values should just follow without any corrections
>> starting at pmd_offset(pudp, addr).
>>
>> The new warning here just informs about the fact that the iteration range is
>> going to be wrong (as well as everything else probably) because the input
>> address is not aligned. 'table = pmd_offset(pudp, 0UL)' will prevent it from
>> hitting unaligned BUG_ON() in pmd_free(). 
>>
>> Using 'pmdp = pmd_offset(pudp, 0UL)' for iterations will be sort of fixing
>> or aligning the wrong input 'addr' which we always wanted to avoid.
> 
> So you want to hide the BUG_ON in pmd_free() by changing this to a
> warning in the caller (pud_free_pmd_page()) and sanitising the value
> passed to pmd_free(). I don't see how this is different from just
> turning the BUG_ON into a warning in pmd_free() directly (which I don't
> think we should, see below).
> 
> It looks to me like we should either fix the callers in ioremap.c (e.g.
> ioremap_try_huge_pud() returning 0 if addr is not PUD_SIZE aligned, not
> just phys_addr) or return 0 in pud_free_pmd_page() with a similar check.
> I'd go for changing ioremap.c since x86 doesn't have such check either.

Sounds good. Will change ioremap_try_huge_[pud|pmd] to include alignment
checks for 'addr' along with existing 'phys_addr'. But still bit inclined
towards having XXX_offset(XXX, 0UL) for fetching the pgtable page address
which is different than XXX_offset(XXX, addr) for iteration purpose unless
if you have concerns.

> 
> IIUC currently if we pass a PUD_SIZE range to ioremap_page_range() where
> phys_addr is PUD_SIZE aligned but the virtual addr is not, we'd end up
> randomly freeing the pmd page that potentially still contains valid
> mappings. Your patch just hides the problem by turning the BUG_ON into a
> warning but doesn't solve it.

Right. This patch tried to use XXX_offset(XXX, 0UL) which is a better way
fetching the pgtable page address but that does not solve the problem related
to unaligned input virtual address. VM_WARN_ONCE() just shifts the problem
bit earlier without solving it as you have pointed out.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel