Message ID | 7baae4086cbb9ffb08c933b065ff7d29dbc03dd6.1596734104.git.christophe.leroy@csgroup.eu (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [v1,1/5] powerpc/mm: sanity_check_fault() should work for all, not only BOOK3S | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (3cd2184115b85cc8242fec3d42529cd112962984) |
snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 26 lines checked |
snowpatch_ozlabs/needsstable | warning | Please consider tagging this patch for stable! |
Excerpts from Christophe Leroy's message of August 7, 2020 3:15 am: > The verification and message introduced by commit 374f3f5979f9 > ("powerpc/mm/hash: Handle user access of kernel address gracefully") > applies to all platforms, it should not be limited to BOOK3S. > > Make the BOOK3S version of sanity_check_fault() the one for all, > and bail out earlier if not BOOK3S. > > Fixes: 374f3f5979f9 ("powerpc/mm/hash: Handle user access of kernel address gracefully") > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > arch/powerpc/mm/fault.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c > index 925a7231abb3..2efa34d7e644 100644 > --- a/arch/powerpc/mm/fault.c > +++ b/arch/powerpc/mm/fault.c > @@ -303,7 +303,6 @@ static inline void cmo_account_page_fault(void) > static inline void cmo_account_page_fault(void) { } > #endif /* CONFIG_PPC_SMLPAR */ > > -#ifdef CONFIG_PPC_BOOK3S > static void sanity_check_fault(bool is_write, bool is_user, > unsigned long error_code, unsigned long address) > { > @@ -320,6 +319,9 @@ static void sanity_check_fault(bool is_write, bool is_user, > return; > } > > + if (!IS_ENABLED(CONFIG_PPC_BOOK3S)) > + return; Seems okay. Why is address == -1 special though? I guess it's because it may not be an exploit kernel reference but a buggy pointer underflow? In that case -1 doesn't seem like it would catch very much. Would it be better to test for high bit set for example ((long)address < 0) ? Anyway for your patch Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > + > /* > * For hash translation mode, we should never get a > * PROTFAULT. Any update to pte to reduce access will result in us > @@ -354,10 +356,6 @@ static void sanity_check_fault(bool is_write, bool is_user, > > WARN_ON_ONCE(error_code & DSISR_PROTFAULT); > } > -#else > -static void sanity_check_fault(bool is_write, bool is_user, > - unsigned long error_code, unsigned long address) { } > -#endif /* CONFIG_PPC_BOOK3S */ > > /* > * Define the correct "is_write" bit in error_code based > -- > 2.25.0 > >
Le 08/09/2020 à 10:43, Nicholas Piggin a écrit : > Excerpts from Christophe Leroy's message of August 7, 2020 3:15 am: >> The verification and message introduced by commit 374f3f5979f9 >> ("powerpc/mm/hash: Handle user access of kernel address gracefully") >> applies to all platforms, it should not be limited to BOOK3S. >> >> Make the BOOK3S version of sanity_check_fault() the one for all, >> and bail out earlier if not BOOK3S. >> >> Fixes: 374f3f5979f9 ("powerpc/mm/hash: Handle user access of kernel address gracefully") >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> arch/powerpc/mm/fault.c | 8 +++----- >> 1 file changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c >> index 925a7231abb3..2efa34d7e644 100644 >> --- a/arch/powerpc/mm/fault.c >> +++ b/arch/powerpc/mm/fault.c >> @@ -303,7 +303,6 @@ static inline void cmo_account_page_fault(void) >> static inline void cmo_account_page_fault(void) { } >> #endif /* CONFIG_PPC_SMLPAR */ >> >> -#ifdef CONFIG_PPC_BOOK3S >> static void sanity_check_fault(bool is_write, bool is_user, >> unsigned long error_code, unsigned long address) >> { >> @@ -320,6 +319,9 @@ static void sanity_check_fault(bool is_write, bool is_user, >> return; >> } >> >> + if (!IS_ENABLED(CONFIG_PPC_BOOK3S)) >> + return; > > Seems okay. Why is address == -1 special though? I guess it's because > it may not be an exploit kernel reference but a buggy pointer underflow? > In that case -1 doesn't seem like it would catch very much. Would it be > better to test for high bit set for example ((long)address < 0) ? See https://github.com/linuxppc/linux/commit/0f9aee0cb9da7db7d96f63cfa2dc5e4f1bffeb87#diff-f9658f412252f3bb3093e0a95b37f3ac -1 is what mmap() returns on error, if the app uses that as a pointer that's a programming error not an exploit. Euh .. If you test (long)address < 0, then the entire kernel falls into that range as usually it goes from 0xc0000000 to 0xffffffff But we could skip the top page entirely, anyway it is never mapped. > > Anyway for your patch > > Reviewed-by: Nicholas Piggin <npiggin@gmail.com> Thanks Christophe
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c index 925a7231abb3..2efa34d7e644 100644 --- a/arch/powerpc/mm/fault.c +++ b/arch/powerpc/mm/fault.c @@ -303,7 +303,6 @@ static inline void cmo_account_page_fault(void) static inline void cmo_account_page_fault(void) { } #endif /* CONFIG_PPC_SMLPAR */ -#ifdef CONFIG_PPC_BOOK3S static void sanity_check_fault(bool is_write, bool is_user, unsigned long error_code, unsigned long address) { @@ -320,6 +319,9 @@ static void sanity_check_fault(bool is_write, bool is_user, return; } + if (!IS_ENABLED(CONFIG_PPC_BOOK3S)) + return; + /* * For hash translation mode, we should never get a * PROTFAULT. Any update to pte to reduce access will result in us @@ -354,10 +356,6 @@ static void sanity_check_fault(bool is_write, bool is_user, WARN_ON_ONCE(error_code & DSISR_PROTFAULT); } -#else -static void sanity_check_fault(bool is_write, bool is_user, - unsigned long error_code, unsigned long address) { } -#endif /* CONFIG_PPC_BOOK3S */ /* * Define the correct "is_write" bit in error_code based
The verification and message introduced by commit 374f3f5979f9 ("powerpc/mm/hash: Handle user access of kernel address gracefully") applies to all platforms, it should not be limited to BOOK3S. Make the BOOK3S version of sanity_check_fault() the one for all, and bail out earlier if not BOOK3S. Fixes: 374f3f5979f9 ("powerpc/mm/hash: Handle user access of kernel address gracefully") Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- arch/powerpc/mm/fault.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)