| Message ID | 20190129003422.9328-4-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Merge text_poke fixes and executable lockdowns | expand |
> Subject: Re: [PATCH v2 03/20] x86/mm: temporary mm struct Subject needs a verb: "Add a temporary... " On Mon, Jan 28, 2019 at 04:34:05PM -0800, Rick Edgecombe wrote: > From: Andy Lutomirski <luto@kernel.org> > > Sometimes we want to set a temporary page-table entries (PTEs) in one of s/a // Also, drop the "we" and make it impartial and passive: "Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behaviour." > the cores, without allowing other cores to use - even speculatively - > these mappings. There are two benefits for doing so: > > (1) Security: if sensitive PTEs are set, temporary mm prevents their use > in other cores. This hardens the security as it prevents exploding a exploding or exploiting? Or exposing? :) > dangling pointer to overwrite sensitive data using the sensitive PTE. > > (2) Avoiding TLB shootdowns: the PTEs do not need to be flushed in > remote page-tables. Those belong in the code comments below, explaining what it is going to be used for. > To do so a temporary mm_struct can be used. Mappings which are private > for this mm can be set in the userspace part of the address-space. > During the whole time in which the temporary mm is loaded, interrupts > must be disabled. > > The first use-case for temporary PTEs, which will follow, is for poking > the kernel text. > > [ Commit message was written by Nadav ] > > Cc: Kees Cook <keescook@chromium.org> > Cc: Dave Hansen <dave.hansen@intel.com> > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> > Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> > Tested-by: Masami Hiramatsu <mhiramat@kernel.org> > Signed-off-by: Andy Lutomirski <luto@kernel.org> > Signed-off-by: Nadav Amit <namit@vmware.com> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > --- > arch/x86/include/asm/mmu_context.h | 32 ++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h > index 19d18fae6ec6..cd0c29e494a6 100644 > --- a/arch/x86/include/asm/mmu_context.h > +++ b/arch/x86/include/asm/mmu_context.h > @@ -356,4 +356,36 @@ static inline unsigned long __get_current_cr3_fast(void) > return cr3; > } > > +typedef struct { Why does it have to be a typedef? That prev.prev below looks unnecessary, instead of just using prev. > + struct mm_struct *prev; Why "prev"? > +} temporary_mm_state_t; That's kinda long - it is longer than the function name below. temp_mm_state_t not enough? > + > +/* > + * Using a temporary mm allows to set temporary mappings that are not accessible > + * by other cores. Such mappings are needed to perform sensitive memory writes > + * that override the kernel memory protections (e.g., W^X), without exposing the > + * temporary page-table mappings that are required for these write operations to > + * other cores. > + * > + * Context: The temporary mm needs to be used exclusively by a single core. To > + * harden security IRQs must be disabled while the temporary mm is ^ , > + * loaded, thereby preventing interrupt handler bugs from override the s/override/overriding/ > + * kernel memory protection. > + */ > +static inline temporary_mm_state_t use_temporary_mm(struct mm_struct *mm) > +{ > + temporary_mm_state_t state; > + > + lockdep_assert_irqs_disabled(); > + state.prev = this_cpu_read(cpu_tlbstate.loaded_mm); > + switch_mm_irqs_off(NULL, mm, current); > + return state; > +} > + > +static inline void unuse_temporary_mm(temporary_mm_state_t prev) > +{ > + lockdep_assert_irqs_disabled(); > + switch_mm_irqs_off(NULL, prev.prev, current); > +} > + > #endif /* _ASM_X86_MMU_CONTEXT_H */ > -- > 2.17.1 >
> On Jan 31, 2019, at 3:29 AM, Borislav Petkov <bp@alien8.de> wrote: > >> Subject: Re: [PATCH v2 03/20] x86/mm: temporary mm struct > > Subject needs a verb: "Add a temporary... " > > On Mon, Jan 28, 2019 at 04:34:05PM -0800, Rick Edgecombe wrote: >> From: Andy Lutomirski <luto@kernel.org> >> >> Sometimes we want to set a temporary page-table entries (PTEs) in one of > > s/a // > > Also, drop the "we" and make it impartial and passive: > > "Describe your changes in imperative mood, e.g. "make xyzzy do frotz" > instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy > to do frotz", as if you are giving orders to the codebase to change > its behaviour." > >> the cores, without allowing other cores to use - even speculatively - >> these mappings. There are two benefits for doing so: >> >> (1) Security: if sensitive PTEs are set, temporary mm prevents their use >> in other cores. This hardens the security as it prevents exploding a > > exploding or exploiting? Or exposing? :) > >> dangling pointer to overwrite sensitive data using the sensitive PTE. >> >> (2) Avoiding TLB shootdowns: the PTEs do not need to be flushed in >> remote page-tables. > > Those belong in the code comments below, explaining what it is going to > be used for. I will add it to the code as well. > >> To do so a temporary mm_struct can be used. Mappings which are private >> for this mm can be set in the userspace part of the address-space. >> During the whole time in which the temporary mm is loaded, interrupts >> must be disabled. >> >> The first use-case for temporary PTEs, which will follow, is for poking >> the kernel text. >> >> [ Commit message was written by Nadav ] >> >> Cc: Kees Cook <keescook@chromium.org> >> Cc: Dave Hansen <dave.hansen@intel.com> >> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> >> Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> >> Tested-by: Masami Hiramatsu <mhiramat@kernel.org> >> Signed-off-by: Andy Lutomirski <luto@kernel.org> >> Signed-off-by: Nadav Amit <namit@vmware.com> >> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> >> --- >> arch/x86/include/asm/mmu_context.h | 32 ++++++++++++++++++++++++++++++ >> 1 file changed, 32 insertions(+) >> >> diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h >> index 19d18fae6ec6..cd0c29e494a6 100644 >> --- a/arch/x86/include/asm/mmu_context.h >> +++ b/arch/x86/include/asm/mmu_context.h >> @@ -356,4 +356,36 @@ static inline unsigned long __get_current_cr3_fast(void) >> return cr3; >> } >> >> +typedef struct { > > Why does it have to be a typedef? Having a different struct can prevent the misuse of using mm_structs in unuse_temporary_mm() that were not “used” using use_temporary_mm. The typedef, I presume, can deter users from starting to play with the internal “private” fields. > That prev.prev below looks unnecessary, instead of just using prev. > >> + struct mm_struct *prev; > > Why "prev”? This is obviously the previous active mm. Feel free to suggest an alternative name. >> +} temporary_mm_state_t; > > That's kinda long - it is longer than the function name below. > temp_mm_state_t not enough? I will change it. > >> + >> +/* >> + * Using a temporary mm allows to set temporary mappings that are not accessible >> + * by other cores. Such mappings are needed to perform sensitive memory writes >> + * that override the kernel memory protections (e.g., W^X), without exposing the >> + * temporary page-table mappings that are required for these write operations to >> + * other cores. >> + * >> + * Context: The temporary mm needs to be used exclusively by a single core. To >> + * harden security IRQs must be disabled while the temporary mm is > ^ > , > >> + * loaded, thereby preventing interrupt handler bugs from override the > > s/override/overriding/ I will fix all of these typos, comment. Thank you. Meta-question: could you please review the entire patch-set? This is actually v9 of this particular patch - it was part of a separate patch-set before. I don’t think that the patch has changed since (the real) v1. These sporadic comments after each version really makes it hard to get this work completed.
On Thu, Jan 31, 2019 at 10:19:54PM +0000, Nadav Amit wrote: > Meta-question: could you please review the entire patch-set? This is > actually v9 of this particular patch - it was part of a separate patch-set > before. I don’t think that the patch has changed since (the real) v1. > > These sporadic comments after each version really makes it hard to get this > work completed. Sorry but where I am the day has only 24 hours and this patchset is not the only one in my overflowing mbox. If my sporadic comments are making it hard to finish your work, I better not interfere then.
> On Jan 31, 2019, at 4:08 PM, Borislav Petkov <bp@alien8.de> wrote: > > On Thu, Jan 31, 2019 at 10:19:54PM +0000, Nadav Amit wrote: >> Meta-question: could you please review the entire patch-set? This is >> actually v9 of this particular patch - it was part of a separate patch-set >> before. I don’t think that the patch has changed since (the real) v1. >> >> These sporadic comments after each version really makes it hard to get this >> work completed. > > Sorry but where I am the day has only 24 hours and this patchset is not > the only one in my overflowing mbox. If my sporadic comments are making > it hard to finish your work, I better not interfere then. I certainly did not intend for it to sound this way, and your feedback is obviously valuable. Just let me know when you are done reviewing the patch-set, so I will not overflow your mailbox with even unnecessary versions of these patches. :)
On Thu, Jan 31, 2019 at 10:19:54PM +0000, Nadav Amit wrote: > Having a different struct can prevent the misuse of using mm_structs in > unuse_temporary_mm() that were not “used” using use_temporary_mm. The > typedef, I presume, can deter users from starting to play with the internal > “private” fields. Ok, makes sense. > > That prev.prev below looks unnecessary, instead of just using prev. > > > >> + struct mm_struct *prev; > > > > Why "prev”? > > This is obviously the previous active mm. Feel free to suggest an > alternative name. Well, when I look at the typedef I'm wondering why is it called "prev" but I guess this is to mean that it will be saving the previously used mm, so ack. Thx.
diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h index 19d18fae6ec6..cd0c29e494a6 100644 --- a/arch/x86/include/asm/mmu_context.h +++ b/arch/x86/include/asm/mmu_context.h @@ -356,4 +356,36 @@ static inline unsigned long __get_current_cr3_fast(void) return cr3; } +typedef struct { + struct mm_struct *prev; +} temporary_mm_state_t; + +/* + * Using a temporary mm allows to set temporary mappings that are not accessible + * by other cores. Such mappings are needed to perform sensitive memory writes + * that override the kernel memory protections (e.g., W^X), without exposing the + * temporary page-table mappings that are required for these write operations to + * other cores. + * + * Context: The temporary mm needs to be used exclusively by a single core. To + * harden security IRQs must be disabled while the temporary mm is + * loaded, thereby preventing interrupt handler bugs from override the + * kernel memory protection. + */ +static inline temporary_mm_state_t use_temporary_mm(struct mm_struct *mm) +{ + temporary_mm_state_t state; + + lockdep_assert_irqs_disabled(); + state.prev = this_cpu_read(cpu_tlbstate.loaded_mm); + switch_mm_irqs_off(NULL, mm, current); + return state; +} + +static inline void unuse_temporary_mm(temporary_mm_state_t prev) +{ + lockdep_assert_irqs_disabled(); + switch_mm_irqs_off(NULL, prev.prev, current); +} + #endif /* _ASM_X86_MMU_CONTEXT_H */