The virt_addr_valid() function is meant to return true iff virt_to_page() will return a valid struct page reference. This is true iff the address provided is found within the unmapped address range between PAGE_OFFSET & MAP_BASE, but we don't currently check for that condition. Instead we simply mask the address to obtain what will be a physical address if the virtual address is indeed in the desired range, shift it to form a PFN & then call pfn_valid(). This can incorrectly return true if called with a virtual address which, after masking, happens to form a physical address corresponding to a valid PFN.
For example we may vmalloc an address in the kernel mapped region starting a MAP_BASE & obtain the virtual address:
addr = 0xc000000000002000
When masked by virt_to_phys(), which uses __pa() & in turn CPHYSADDR(), we obtain the following (bogus) physical address:
addr = 0x2000
In a common system with PHYS_OFFSET=0 this will correspond to a valid struct page which should really be accessed by virtual address PAGE_OFFSET+0x2000, causing virt_addr_valid() to incorrectly return 1 indicating that the original address corresponds to a struct page.
This is equivalent to the ARM64 change made in commit ca219452c6b8 ("arm64: Correctly bounds check virt_addr_valid").
This fixes fallout when hardened usercopy is enabled caused by the related commit 517e1fbeb65f ("mm/usercopy: Drop extra is_vmalloc_or_module() check") which removed a check for the vmalloc range that was present from the introduction of the hardened usercopy feature.
Signed-off-by: Paul Burton paul.burton@mips.com References: ca219452c6b8 ("arm64: Correctly bounds check virt_addr_valid") References: 517e1fbeb65f ("mm/usercopy: Drop extra is_vmalloc_or_module() check") Reported-by: Julien Cristau jcristau@debian.org Tested-by: YunQiang Su ysu@wavecomp.com URL: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=929366 Cc: stable@vger.kernel.org # v4.12+ ---
arch/mips/mm/mmap.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c index 2f616ebeb7e0..7755a1fad05a 100644 --- a/arch/mips/mm/mmap.c +++ b/arch/mips/mm/mmap.c @@ -203,6 +203,11 @@ unsigned long arch_randomize_brk(struct mm_struct *mm)
int __virt_addr_valid(const volatile void *kaddr) { + unsigned long vaddr = (unsigned long)vaddr; + + if ((vaddr < PAGE_OFFSET) || (vaddr >= MAP_BASE)) + return 0; + return pfn_valid(PFN_DOWN(virt_to_phys(kaddr))); } EXPORT_SYMBOL_GPL(__virt_addr_valid);
On 5/28/19 7:05 PM, Paul Burton wrote:
The virt_addr_valid() function is meant to return true iff virt_to_page() will return a valid struct page reference. This is true iff the address provided is found within the unmapped address range between PAGE_OFFSET & MAP_BASE, but we don't currently check for that condition. Instead we simply mask the address to obtain what will be a physical address if the virtual address is indeed in the desired range, shift it to form a PFN & then call pfn_valid(). This can incorrectly return true if called with a virtual address which, after masking, happens to form a physical address corresponding to a valid PFN.
For example we may vmalloc an address in the kernel mapped region starting a MAP_BASE & obtain the virtual address:
addr = 0xc000000000002000
When masked by virt_to_phys(), which uses __pa() & in turn CPHYSADDR(), we obtain the following (bogus) physical address:
addr = 0x2000
In a common system with PHYS_OFFSET=0 this will correspond to a valid struct page which should really be accessed by virtual address PAGE_OFFSET+0x2000, causing virt_addr_valid() to incorrectly return 1 indicating that the original address corresponds to a struct page.
This is equivalent to the ARM64 change made in commit ca219452c6b8 ("arm64: Correctly bounds check virt_addr_valid").
This fixes fallout when hardened usercopy is enabled caused by the related commit 517e1fbeb65f ("mm/usercopy: Drop extra is_vmalloc_or_module() check") which removed a check for the vmalloc range that was present from the introduction of the hardened usercopy feature.
Signed-off-by: Paul Burton paul.burton@mips.com References: ca219452c6b8 ("arm64: Correctly bounds check virt_addr_valid") References: 517e1fbeb65f ("mm/usercopy: Drop extra is_vmalloc_or_module() check") Reported-by: Julien Cristau jcristau@debian.org Tested-by: YunQiang Su ysu@wavecomp.com
Reviewed-by: Philippe Mathieu-Daudé f4bug@amsat.org
URL: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=929366 Cc: stable@vger.kernel.org # v4.12+
arch/mips/mm/mmap.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c index 2f616ebeb7e0..7755a1fad05a 100644 --- a/arch/mips/mm/mmap.c +++ b/arch/mips/mm/mmap.c @@ -203,6 +203,11 @@ unsigned long arch_randomize_brk(struct mm_struct *mm) int __virt_addr_valid(const volatile void *kaddr) {
- unsigned long vaddr = (unsigned long)vaddr;
- if ((vaddr < PAGE_OFFSET) || (vaddr >= MAP_BASE))
return 0;
- return pfn_valid(PFN_DOWN(virt_to_phys(kaddr)));
} EXPORT_SYMBOL_GPL(__virt_addr_valid);
Hello,
Paul Burton wrote:
The virt_addr_valid() function is meant to return true iff virt_to_page() will return a valid struct page reference. This is true iff the address provided is found within the unmapped address range between PAGE_OFFSET & MAP_BASE, but we don't currently check for that condition. Instead we simply mask the address to obtain what will be a physical address if the virtual address is indeed in the desired range, shift it to form a PFN & then call pfn_valid(). This can incorrectly return true if called with a virtual address which, after masking, happens to form a physical address corresponding to a valid PFN.
For example we may vmalloc an address in the kernel mapped region starting a MAP_BASE & obtain the virtual address:
addr = 0xc000000000002000
When masked by virt_to_phys(), which uses __pa() & in turn CPHYSADDR(), we obtain the following (bogus) physical address:
addr = 0x2000
In a common system with PHYS_OFFSET=0 this will correspond to a valid struct page which should really be accessed by virtual address PAGE_OFFSET+0x2000, causing virt_addr_valid() to incorrectly return 1 indicating that the original address corresponds to a struct page.
This is equivalent to the ARM64 change made in commit ca219452c6b8 ("arm64: Correctly bounds check virt_addr_valid").
This fixes fallout when hardened usercopy is enabled caused by the related commit 517e1fbeb65f ("mm/usercopy: Drop extra is_vmalloc_or_module() check") which removed a check for the vmalloc range that was present from the introduction of the hardened usercopy feature.
Signed-off-by: Paul Burton paul.burton@mips.com References: ca219452c6b8 ("arm64: Correctly bounds check virt_addr_valid") References: 517e1fbeb65f ("mm/usercopy: Drop extra is_vmalloc_or_module() check") Reported-by: Julien Cristau jcristau@debian.org Tested-by: YunQiang Su ysu@wavecomp.com URL: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=929366 Cc: stable@vger.kernel.org # v4.12+ Reviewed-by: Philippe Mathieu-Daudé f4bug@amsat.org
Series applied to mips-fixes.
Thanks, Paul
[ This message was auto-generated; if you believe anything is incorrect then please email paul.burton@mips.com to report it. ]
On 5/28/19 7:05 PM, Paul Burton wrote:
The virt_addr_valid() function is meant to return true iff virt_to_page() will return a valid struct page reference. This is true iff the address provided is found within the unmapped address range between PAGE_OFFSET & MAP_BASE, but we don't currently check for that condition. Instead we simply mask the address to obtain what will be a physical address if the virtual address is indeed in the desired range, shift it to form a PFN & then call pfn_valid(). This can incorrectly return true if called with a virtual address which, after masking, happens to form a physical address corresponding to a valid PFN.
For example we may vmalloc an address in the kernel mapped region starting a MAP_BASE & obtain the virtual address:
addr = 0xc000000000002000
When masked by virt_to_phys(), which uses __pa() & in turn CPHYSADDR(), we obtain the following (bogus) physical address:
addr = 0x2000
In a common system with PHYS_OFFSET=0 this will correspond to a valid struct page which should really be accessed by virtual address PAGE_OFFSET+0x2000, causing virt_addr_valid() to incorrectly return 1 indicating that the original address corresponds to a struct page.
This is equivalent to the ARM64 change made in commit ca219452c6b8 ("arm64: Correctly bounds check virt_addr_valid").
This fixes fallout when hardened usercopy is enabled caused by the related commit 517e1fbeb65f ("mm/usercopy: Drop extra is_vmalloc_or_module() check") which removed a check for the vmalloc range that was present from the introduction of the hardened usercopy feature.
Signed-off-by: Paul Burton paul.burton@mips.com References: ca219452c6b8 ("arm64: Correctly bounds check virt_addr_valid") References: 517e1fbeb65f ("mm/usercopy: Drop extra is_vmalloc_or_module() check") Reported-by: Julien Cristau jcristau@debian.org Tested-by: YunQiang Su ysu@wavecomp.com URL: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=929366 Cc: stable@vger.kernel.org # v4.12+
arch/mips/mm/mmap.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c index 2f616ebeb7e0..7755a1fad05a 100644 --- a/arch/mips/mm/mmap.c +++ b/arch/mips/mm/mmap.c @@ -203,6 +203,11 @@ unsigned long arch_randomize_brk(struct mm_struct *mm) int __virt_addr_valid(const volatile void *kaddr) {
- unsigned long vaddr = (unsigned long)vaddr;
- if ((vaddr < PAGE_OFFSET) || (vaddr >= MAP_BASE))
return 0;
- return pfn_valid(PFN_DOWN(virt_to_phys(kaddr)));
} EXPORT_SYMBOL_GPL(__virt_addr_valid);
Hi Paul,
Someone complained that this compiled to a constant "return 0" for him: https://bugs.openwrt.org/index.php?do=details&task_id=2305#comment6554
I just checked this on a unmodified 5.2-rc4 with the xway_defconfig and I get this:
0001915c <__virt_addr_valid>: 1915c: 03e00008 jr ra 19160: 00001025 move v0,zero
Is this intended?
Hauke
On Tue, Jun 11, 2019 at 01:41:21AM +0200, Hauke Mehrtens wrote:
On 5/28/19 7:05 PM, Paul Burton wrote:
arch/mips/mm/mmap.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c index 2f616ebeb7e0..7755a1fad05a 100644 --- a/arch/mips/mm/mmap.c +++ b/arch/mips/mm/mmap.c @@ -203,6 +203,11 @@ unsigned long arch_randomize_brk(struct mm_struct *mm) int __virt_addr_valid(const volatile void *kaddr) {
- unsigned long vaddr = (unsigned long)vaddr;
the second vaddr should be better kaddr
- if ((vaddr < PAGE_OFFSET) || (vaddr >= MAP_BASE))
return 0;
- return pfn_valid(PFN_DOWN(virt_to_phys(kaddr)));
} EXPORT_SYMBOL_GPL(__virt_addr_valid);
Someone complained that this compiled to a constant "return 0" for him: https://bugs.openwrt.org/index.php?do=details&task_id=2305#comment6554
I just checked this on a unmodified 5.2-rc4 with the xway_defconfig and I get this:
0001915c <__virt_addr_valid>: 1915c: 03e00008 jr ra 19160: 00001025 move v0,zero
Is this intended?
I don't think so. Interesting what the compiler decides to do here.
Thomas.
Hi Hauke & Thomas,
On Tue, Jun 11, 2019 at 10:19:47AM +0200, Thomas Bogendoerfer wrote:
On Tue, Jun 11, 2019 at 01:41:21AM +0200, Hauke Mehrtens wrote:
On 5/28/19 7:05 PM, Paul Burton wrote:
diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c index 2f616ebeb7e0..7755a1fad05a 100644 --- a/arch/mips/mm/mmap.c +++ b/arch/mips/mm/mmap.c @@ -203,6 +203,11 @@ unsigned long arch_randomize_brk(struct mm_struct *mm) int __virt_addr_valid(const volatile void *kaddr) {
- unsigned long vaddr = (unsigned long)vaddr;
the second vaddr should be better kaddr
D'oh..! Right you are...
Returning false all the time is enough to silence the hardened usercopy warnings but clearly not the right behaviour.
Someone complained that this compiled to a constant "return 0" for him: https://bugs.openwrt.org/index.php?do=details&task_id=2305#comment6554
I just checked this on a unmodified 5.2-rc4 with the xway_defconfig and I get this:
0001915c <__virt_addr_valid>: 1915c: 03e00008 jr ra 19160: 00001025 move v0,zero
Is this intended?
I don't think so. Interesting what the compiler decides to do here.
Yes, this is equivalent to using uninitialized_var() but I'm surprised the code got discarded entirely...
Thanks, Paul
linux-stable-mirror@lists.linaro.org