[PATCH] arm64: Correctly bounds check virt_addr_valid

[PATCH] arm64: Correctly bounds check virt_addr_valid

[Laura Abbott]

virt_addr_valid is supposed to return true if and only if virt_to_page
returns a valid page structure. The current macro does math on whatever
address is given and passes that to pfn_valid to verify. vmalloc and
module addresses can happen to generate a pfn that 'happens' to be
valid. Fix this by only performing the pfn_valid check on addresses that
have the potential to be valid.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
This caused a bug at least twice in hardened usercopy so it is an
actual problem. A further TODO is full DEBUG_VIRTUAL support to
catch these types of mistakes.
---
 arch/arm64/include/asm/memory.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index 31b7322..f741e19 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -214,7 +214,7 @@ static inline void *phys_to_virt(phys_addr_t x)
 
 #ifndef CONFIG_SPARSEMEM_VMEMMAP
 #define virt_to_page(kaddr)	pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
-#define virt_addr_valid(kaddr)	pfn_valid(__pa(kaddr) >> PAGE_SHIFT)
+#define virt_addr_valid(kaddr)	(((u64)kaddr) >= PAGE_OFFSET && pfn_valid(__pa(kaddr) >> PAGE_SHIFT))
 #else
 #define __virt_to_pgoff(kaddr)	(((u64)(kaddr) & ~PAGE_OFFSET) / PAGE_SIZE * sizeof(struct page))
 #define __page_to_voff(kaddr)	(((u64)(page) & ~VMEMMAP_START) * PAGE_SIZE / sizeof(struct page))
@@ -222,8 +222,8 @@ static inline void *phys_to_virt(phys_addr_t x)
 #define page_to_virt(page)	((void *)((__page_to_voff(page)) | PAGE_OFFSET))
 #define virt_to_page(vaddr)	((struct page *)((__virt_to_pgoff(vaddr)) | VMEMMAP_START))
 
-#define virt_addr_valid(kaddr)	pfn_valid((((u64)(kaddr) & ~PAGE_OFFSET) \
-					   + PHYS_OFFSET) >> PAGE_SHIFT)
+#define virt_addr_valid(kaddr)	(((u64)kaddr) >= PAGE_OFFSET && pfn_valid((((u64)(kaddr) & ~PAGE_OFFSET) \
+					   + PHYS_OFFSET) >> PAGE_SHIFT))
 #endif
 #endif
 
-- 
2.7.4

Re: [PATCH] arm64: Correctly bounds check virt_addr_valid

[Laura Abbott]

On 09/21/2016 10:28 AM, Laura Abbott wrote:
> virt_addr_valid is supposed to return true if and only if virt_to_page
> returns a valid page structure. The current macro does math on whatever
> address is given and passes that to pfn_valid to verify. vmalloc and
> module addresses can happen to generate a pfn that 'happens' to be
> valid. Fix this by only performing the pfn_valid check on addresses that
> have the potential to be valid.
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> This caused a bug at least twice in hardened usercopy so it is an
> actual problem. A further TODO is full DEBUG_VIRTUAL support to
> catch these types of mistakes.
> ---
>  arch/arm64/include/asm/memory.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index 31b7322..f741e19 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -214,7 +214,7 @@ static inline void *phys_to_virt(phys_addr_t x)
>
>  #ifndef CONFIG_SPARSEMEM_VMEMMAP
>  #define virt_to_page(kaddr)	pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
> -#define virt_addr_valid(kaddr)	pfn_valid(__pa(kaddr) >> PAGE_SHIFT)
> +#define virt_addr_valid(kaddr)	(((u64)kaddr) >= PAGE_OFFSET && pfn_valid(__pa(kaddr) >> PAGE_SHIFT))
>  #else
>  #define __virt_to_pgoff(kaddr)	(((u64)(kaddr) & ~PAGE_OFFSET) / PAGE_SIZE * sizeof(struct page))
>  #define __page_to_voff(kaddr)	(((u64)(page) & ~VMEMMAP_START) * PAGE_SIZE / sizeof(struct page))
> @@ -222,8 +222,8 @@ static inline void *phys_to_virt(phys_addr_t x)
>  #define page_to_virt(page)	((void *)((__page_to_voff(page)) | PAGE_OFFSET))
>  #define virt_to_page(vaddr)	((struct page *)((__virt_to_pgoff(vaddr)) | VMEMMAP_START))
>
> -#define virt_addr_valid(kaddr)	pfn_valid((((u64)(kaddr) & ~PAGE_OFFSET) \
> -					   + PHYS_OFFSET) >> PAGE_SHIFT)
> +#define virt_addr_valid(kaddr)	(((u64)kaddr) >= PAGE_OFFSET && pfn_valid((((u64)(kaddr) & ~PAGE_OFFSET) \
> +					   + PHYS_OFFSET) >> PAGE_SHIFT))
>  #endif
>  #endif
>
>

Bah, I realized I butchered the macro parenthesization. I'll fix that
in a v2. I'll wait for comments on this first.

Thanks,
Laura

Re: [PATCH] arm64: Correctly bounds check virt_addr_valid

[Mark Rutland]

Hi,

On Wed, Sep 21, 2016 at 10:28:48AM -0700, Laura Abbott wrote:
> virt_addr_valid is supposed to return true if and only if virt_to_page
> returns a valid page structure. The current macro does math on whatever
> address is given and passes that to pfn_valid to verify. vmalloc and
> module addresses can happen to generate a pfn that 'happens' to be
> valid. Fix this by only performing the pfn_valid check on addresses that
> have the potential to be valid.
> 
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> This caused a bug at least twice in hardened usercopy so it is an
> actual problem.

Are there other potentially-broken users of virt_addr_valid? It's not
clear to me what some drivers are doing with this, and therefore whether
we need to cc stable.

> A further TODO is full DEBUG_VIRTUAL support to
> catch these types of mistakes.
> ---
>  arch/arm64/include/asm/memory.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index 31b7322..f741e19 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -214,7 +214,7 @@ static inline void *phys_to_virt(phys_addr_t x)
>  
>  #ifndef CONFIG_SPARSEMEM_VMEMMAP
>  #define virt_to_page(kaddr)	pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
> -#define virt_addr_valid(kaddr)	pfn_valid(__pa(kaddr) >> PAGE_SHIFT)
> +#define virt_addr_valid(kaddr)	(((u64)kaddr) >= PAGE_OFFSET && pfn_valid(__pa(kaddr) >> PAGE_SHIFT))
>  #else
>  #define __virt_to_pgoff(kaddr)	(((u64)(kaddr) & ~PAGE_OFFSET) / PAGE_SIZE * sizeof(struct page))
>  #define __page_to_voff(kaddr)	(((u64)(page) & ~VMEMMAP_START) * PAGE_SIZE / sizeof(struct page))
> @@ -222,8 +222,8 @@ static inline void *phys_to_virt(phys_addr_t x)
>  #define page_to_virt(page)	((void *)((__page_to_voff(page)) | PAGE_OFFSET))
>  #define virt_to_page(vaddr)	((struct page *)((__virt_to_pgoff(vaddr)) | VMEMMAP_START))
>  
> -#define virt_addr_valid(kaddr)	pfn_valid((((u64)(kaddr) & ~PAGE_OFFSET) \
> -					   + PHYS_OFFSET) >> PAGE_SHIFT)
> +#define virt_addr_valid(kaddr)	(((u64)kaddr) >= PAGE_OFFSET && pfn_valid((((u64)(kaddr) & ~PAGE_OFFSET) \
> +					   + PHYS_OFFSET) >> PAGE_SHIFT))
>  #endif
>  #endif

Given the common sub-expression, perhaps it would be better to leave
these as-is, but prefix them with '_', and after the #endif, have
something like:

#define _virt_addr_is_linear(kaddr)	(((u64)(kaddr)) >= PAGE_OFFSET)
#define virt_addr_valid(kaddr)		(_virt_addr_is_linear(kaddr) && _virt_addr_valid(kaddr))

Otherwise, modulo the parenthesis issue you mentioned, this looks
logically correct to me.

Thanks,
Mark.

Re: [PATCH] arm64: Correctly bounds check virt_addr_valid

[Laura Abbott]

On 09/21/2016 10:58 AM, Mark Rutland wrote:
> Hi,
>
> On Wed, Sep 21, 2016 at 10:28:48AM -0700, Laura Abbott wrote:
>> virt_addr_valid is supposed to return true if and only if virt_to_page
>> returns a valid page structure. The current macro does math on whatever
>> address is given and passes that to pfn_valid to verify. vmalloc and
>> module addresses can happen to generate a pfn that 'happens' to be
>> valid. Fix this by only performing the pfn_valid check on addresses that
>> have the potential to be valid.
>>
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>> This caused a bug at least twice in hardened usercopy so it is an
>> actual problem.
>
> Are there other potentially-broken users of virt_addr_valid? It's not
> clear to me what some drivers are doing with this, and therefore whether
> we need to cc stable.
>

The number of users is pretty limited. Some of them use it as a debugging
check, others are using it more like hardened usercopy. The number of
users that would actually affect arm64 seems so small I don't think it's
worth trying to backport to stable. Hardened usercopy was getting hit
particularly hard because usercopy was happening on all types of memory
whereas the drivers tend to be more limited in scope.

>> A further TODO is full DEBUG_VIRTUAL support to
>> catch these types of mistakes.
>> ---
>>  arch/arm64/include/asm/memory.h | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
>> index 31b7322..f741e19 100644
>> --- a/arch/arm64/include/asm/memory.h
>> +++ b/arch/arm64/include/asm/memory.h
>> @@ -214,7 +214,7 @@ static inline void *phys_to_virt(phys_addr_t x)
>>
>>  #ifndef CONFIG_SPARSEMEM_VMEMMAP
>>  #define virt_to_page(kaddr)	pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
>> -#define virt_addr_valid(kaddr)	pfn_valid(__pa(kaddr) >> PAGE_SHIFT)
>> +#define virt_addr_valid(kaddr)	(((u64)kaddr) >= PAGE_OFFSET && pfn_valid(__pa(kaddr) >> PAGE_SHIFT))
>>  #else
>>  #define __virt_to_pgoff(kaddr)	(((u64)(kaddr) & ~PAGE_OFFSET) / PAGE_SIZE * sizeof(struct page))
>>  #define __page_to_voff(kaddr)	(((u64)(page) & ~VMEMMAP_START) * PAGE_SIZE / sizeof(struct page))
>> @@ -222,8 +222,8 @@ static inline void *phys_to_virt(phys_addr_t x)
>>  #define page_to_virt(page)	((void *)((__page_to_voff(page)) | PAGE_OFFSET))
>>  #define virt_to_page(vaddr)	((struct page *)((__virt_to_pgoff(vaddr)) | VMEMMAP_START))
>>
>> -#define virt_addr_valid(kaddr)	pfn_valid((((u64)(kaddr) & ~PAGE_OFFSET) \
>> -					   + PHYS_OFFSET) >> PAGE_SHIFT)
>> +#define virt_addr_valid(kaddr)	(((u64)kaddr) >= PAGE_OFFSET && pfn_valid((((u64)(kaddr) & ~PAGE_OFFSET) \
>> +					   + PHYS_OFFSET) >> PAGE_SHIFT))
>>  #endif
>>  #endif
>
> Given the common sub-expression, perhaps it would be better to leave
> these as-is, but prefix them with '_', and after the #endif, have
> something like:
>
> #define _virt_addr_is_linear(kaddr)	(((u64)(kaddr)) >= PAGE_OFFSET)
> #define virt_addr_valid(kaddr)		(_virt_addr_is_linear(kaddr) && _virt_addr_valid(kaddr))
>

Good suggestion.

> Otherwise, modulo the parenthesis issue you mentioned, this looks
> logically correct to me.
>
> Thanks,
> Mark.
>

Thanks,
Laura

Re: [PATCH] arm64: Correctly bounds check virt_addr_valid

[Mark Rutland]

On Wed, Sep 21, 2016 at 12:34:46PM -0700, Laura Abbott wrote:
> On 09/21/2016 10:58 AM, Mark Rutland wrote:
> >Are there other potentially-broken users of virt_addr_valid? It's not
> >clear to me what some drivers are doing with this, and therefore whether
> >we need to cc stable.
> 
> The number of users is pretty limited. Some of them use it as a debugging
> check, others are using it more like hardened usercopy. The number of
> users that would actually affect arm64 seems so small I don't think it's
> worth trying to backport to stable.

Ok.

> Hardened usercopy was getting hit particularly hard because usercopy was
> happening on all types of memory whereas the drivers tend to be more limited
> in scope.

Sure.

> >Given the common sub-expression, perhaps it would be better to leave
> >these as-is, but prefix them with '_', and after the #endif, have
> >something like:
> >
> >#define _virt_addr_is_linear(kaddr)	(((u64)(kaddr)) >= PAGE_OFFSET)
> >#define virt_addr_valid(kaddr)		(_virt_addr_is_linear(kaddr) && _virt_addr_valid(kaddr))
> >
> 
> Good suggestion.

FWIW, with that, feel free to add:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.