| Message ID | 20190528170444.1557-1-paul.burton@mips.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 074a1e1167afd82c26f6d03a9a8b997d564bb241 |
| Headers | show |
| Series | [1/2] MIPS: Bounds check virt_addr_valid | expand |
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
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);
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(+)