| Message ID | 20201110162211.9207-8-yu-cheng.yu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Control-flow Enforcement: Shadow Stack | expand |
On Tue, Nov 10, 2020 at 08:21:52AM -0800, Yu-cheng Yu wrote: > Kernel read-only PTEs are setup as _PAGE_DIRTY_HW. Since these become > shadow stack PTEs, remove the dirty bit. This commit message is laconic to say the least. You need to start explaining what you're doing because everytime I look at a patch of yours, I'm always grepping the SDM and looking forward in the patchset, trying to rhyme up what that is all about. Like for this one. I had to fast-forward to the next patch where all that is explained. But this is not how review works - each patch's commit message needs to be understandable on its own because when they land upstream, they're not in a patchset like here. And review should be done in the order the patches are numbered - not by jumping back'n'forth. So please think of the readers of your patches when writing those commit messages. Latter are *not* write-only and not unimportant. And those readers haven't spent copious amounts of time on the technology so being more verbose and explaining things is a Good Thing(tm). Don't worry about explaining too much - better too much than too little. And last but not least, having understandable and properly written commit messages increases the chances of your patches landing upstream considerably. Thx.
On 12/7/2020 8:36 AM, Borislav Petkov wrote: > On Tue, Nov 10, 2020 at 08:21:52AM -0800, Yu-cheng Yu wrote: >> Kernel read-only PTEs are setup as _PAGE_DIRTY_HW. Since these become >> shadow stack PTEs, remove the dirty bit. > > This commit message is laconic to say the least. You need to start > explaining what you're doing because everytime I look at a patch of > yours, I'm always grepping the SDM and looking forward in the patchset, > trying to rhyme up what that is all about. > > Like for this one. I had to fast-forward to the next patch where all > that is explained. But this is not how review works - each patch's > commit message needs to be understandable on its own because when > they land upstream, they're not in a patchset like here. And review > should be done in the order the patches are numbered - not by jumping > back'n'forth. > > So please think of the readers of your patches when writing those commit > messages. Latter are *not* write-only and not unimportant. > > And those readers haven't spent copious amounts of time on the > technology so being more verbose and explaining things is a Good > Thing(tm). Don't worry about explaining too much - better too much than > too little. > > And last but not least, having understandable and properly written > commit messages increases the chances of your patches landing upstream > considerably. > > Thx. > Thanks for your feedback. I will improve the commit logs. -- Yu-cheng
diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h index 810eb1567050..7462a574fc93 100644 --- a/arch/x86/include/asm/pgtable_types.h +++ b/arch/x86/include/asm/pgtable_types.h @@ -193,10 +193,10 @@ enum page_cache_mode { #define _KERNPG_TABLE (__PP|__RW| 0|___A| 0|___D| 0| 0| _ENC) #define _PAGE_TABLE_NOENC (__PP|__RW|_USR|___A| 0|___D| 0| 0) #define _PAGE_TABLE (__PP|__RW|_USR|___A| 0|___D| 0| 0| _ENC) -#define __PAGE_KERNEL_RO (__PP| 0| 0|___A|__NX|___D| 0|___G) -#define __PAGE_KERNEL_ROX (__PP| 0| 0|___A| 0|___D| 0|___G) +#define __PAGE_KERNEL_RO (__PP| 0| 0|___A|__NX| 0| 0|___G) +#define __PAGE_KERNEL_ROX (__PP| 0| 0|___A| 0| 0| 0|___G) #define __PAGE_KERNEL_NOCACHE (__PP|__RW| 0|___A|__NX|___D| 0|___G| __NC) -#define __PAGE_KERNEL_VVAR (__PP| 0|_USR|___A|__NX|___D| 0|___G) +#define __PAGE_KERNEL_VVAR (__PP| 0|_USR|___A|__NX| 0| 0|___G) #define __PAGE_KERNEL_LARGE (__PP|__RW| 0|___A|__NX|___D|_PSE|___G) #define __PAGE_KERNEL_LARGE_EXEC (__PP|__RW| 0|___A| 0|___D|_PSE|___G) #define __PAGE_KERNEL_WP (__PP|__RW| 0|___A|__NX|___D| 0|___G| __WP) diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c index 40baa90e74f4..207bbf796f5f 100644 --- a/arch/x86/mm/pat/set_memory.c +++ b/arch/x86/mm/pat/set_memory.c @@ -1932,7 +1932,7 @@ int set_memory_nx(unsigned long addr, int numpages) int set_memory_ro(unsigned long addr, int numpages) { - return change_page_attr_clear(&addr, numpages, __pgprot(_PAGE_RW), 0); + return change_page_attr_clear(&addr, numpages, __pgprot(_PAGE_RW | _PAGE_DIRTY_HW), 0); } int set_memory_rw(unsigned long addr, int numpages)
Kernel read-only PTEs are setup as _PAGE_DIRTY_HW. Since these become shadow stack PTEs, remove the dirty bit. Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: Kees Cook <keescook@chromium.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Dave Hansen <dave.hansen@linux.intel.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Andy Lutomirski <luto@kernel.org> Cc: Ingo Molnar <mingo@redhat.com> Cc: Borislav Petkov <bp@alien8.de> Cc: Peter Zijlstra <peterz@infradead.org> --- arch/x86/include/asm/pgtable_types.h | 6 +++--- arch/x86/mm/pat/set_memory.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-)