Re: [PATCH v2 05/20] x86/alternative: initializing temporary mm for patching
> On Feb 11, 2019, at 2:47 PM, Andy Lutomirski wrote:
>
> On Mon, Feb 11, 2019 at 11:18 AM Nadav Amit wrote:
>>
>> +
>> + /*
>> +* If breakpoints are enabled, disable them while the temporary mm is
>> +* used - they do not belong and might cause wrong signals or
>> crashes.
>> +*/
>
> Maybe clarify this? Add some mention that the specific problem is
> that user code could set a watchpoint on an address that is also used
> in the temporary mm.
>
> Arguably we should not disable *kernel* breakpoints a la perf, but
> that seems like quite a minor issue, at least as long as
> use_temporary_mm() doesn't get wider use. But a comment that this
> also disables perf breakpoints and that this could be undesirable
> might be in order as well.
I think that in the future there may also be security benefits for disabling
breakpoints when you are in a sensitive code-block, for instance when you
poke text, to prevent the control flow from being hijacked (by exploiting a
bug in the debug exception handler). Some additional steps need to be taken
for that to be beneficial so I leave it out of the comment for now.
Anyhow, how about this:
-- >8 --
From: Nadav Amit
Date: Mon, 11 Feb 2019 03:07:08 -0800
Subject: [PATCH] x86/mm: Save DRs when loading a temporary mm
Prevent user watchpoints from mistakenly firing while the temporary mm
is being used. As the addresses that of the temporary mm might overlap
those of the user-process, this is necessary to prevent wrong signals
or worse things from happening.
Cc: Andy Lutomirski
Signed-off-by: Nadav Amit
---
arch/x86/include/asm/mmu_context.h | 25 +
1 file changed, 25 insertions(+)
diff --git a/arch/x86/include/asm/mmu_context.h
b/arch/x86/include/asm/mmu_context.h
index d684b954f3c0..0d6c72ece750 100644
--- a/arch/x86/include/asm/mmu_context.h
+++ b/arch/x86/include/asm/mmu_context.h
@@ -13,6 +13,7 @@
#include
#include
#include
+#include
extern atomic64_t last_mm_ctx_id;
@@ -358,6 +359,7 @@ static inline unsigned long __get_current_cr3_fast(void)
typedef struct {
struct mm_struct *prev;
+ unsigned short bp_enabled : 1;
} temp_mm_state_t;
/*
@@ -380,6 +382,22 @@ static inline temp_mm_state_t use_temporary_mm(struct
mm_struct *mm)
lockdep_assert_irqs_disabled();
state.prev = this_cpu_read(cpu_tlbstate.loaded_mm);
switch_mm_irqs_off(NULL, mm, current);
+
+ /*
+* If breakpoints are enabled, disable them while the temporary mm is
+* used. Userspace might set up watchpoints on addresses that are used
+* in the temporary mm, which would lead to wrong signals being sent or
+* crashes.
+*
+* Note that breakpoints are not disabled selectively, which also causes
+* kernel breakpoints (e.g., perf's) to be disabled. This might be
+* undesirable, but still seems reasonable as the code that runs in the
+* temporary mm should be short.
+*/
+ state.bp_enabled = hw_breakpoint_active();
+ if (state.bp_enabled)
+ hw_breakpoint_disable();
+
return state;
}
@@ -387,6 +405,13 @@ static inline void unuse_temporary_mm(temp_mm_state_t prev)
{
lockdep_assert_irqs_disabled();
switch_mm_irqs_off(NULL, prev.prev, current);
+
+ /*
+* Restore the breakpoints if they were disabled before the temporary mm
+* was loaded.
+*/
+ if (prev.bp_enabled)
+ hw_breakpoint_restore();
}
#endif /* _ASM_X86_MMU_CONTEXT_H */
--
2.17.1
Re: [PATCH v2 05/20] x86/alternative: initializing temporary mm for patching
On Mon, Feb 11, 2019 at 11:18 AM Nadav Amit wrote:
>
> > On Feb 11, 2019, at 11:07 AM, Andy Lutomirski wrote:
> >
> > I'm certainly amenable to other solutions, but this one does seem the
> > least messy. I looked at my old patch, and it doesn't do what you
> > want. I'd suggest you just add a percpu variable like cpu_dr7 and rig
> > up some accessors so that it stays up to date. Then you can skip the
> > dr7 writes if there are no watchpoints set.
> >
> > Also, EFI is probably a less interesting example than rare_write.
> > With rare_write, especially the dynamically allocated variants that
> > people keep coming up with, we'll need a swath of address space fully
> > as large as the vmalloc area. and getting *that* right while still
> > using the kernel address range might be more of a mess than we really
> > want to deal with.
>
> As long as you feel comfortable with this solution, I’m fine with it.
>
> Here is what I have (untested). I prefer to save/restore all the DRs,
> because IIRC DR6 indications are updated even if breakpoints are disabled
> (in DR7). And anyhow, that is the standard interface.
Seems reasonable, but:
>
>
> -- >8 --
>
> From: Nadav Amit
> Date: Mon, 11 Feb 2019 03:07:08 -0800
> Subject: [PATCH] mm: save DRs when loading temporary mm
>
> Signed-off-by: Nadav Amit
> ---
> arch/x86/include/asm/mmu_context.h | 18 ++
> 1 file changed, 18 insertions(+)
>
> diff --git a/arch/x86/include/asm/mmu_context.h
> b/arch/x86/include/asm/mmu_context.h
> index d684b954f3c0..4f92ec3df149 100644
> --- a/arch/x86/include/asm/mmu_context.h
> +++ b/arch/x86/include/asm/mmu_context.h
> @@ -13,6 +13,7 @@
> #include
> #include
> #include
> +#include
>
> extern atomic64_t last_mm_ctx_id;
>
> @@ -358,6 +359,7 @@ static inline unsigned long __get_current_cr3_fast(void)
>
> typedef struct {
> struct mm_struct *prev;
> + unsigned short bp_enabled : 1;
> } temp_mm_state_t;
>
> /*
> @@ -380,6 +382,15 @@ static inline temp_mm_state_t use_temporary_mm(struct
> mm_struct *mm)
> lockdep_assert_irqs_disabled();
> state.prev = this_cpu_read(cpu_tlbstate.loaded_mm);
> switch_mm_irqs_off(NULL, mm, current);
> +
> + /*
> +* If breakpoints are enabled, disable them while the temporary mm is
> +* used - they do not belong and might cause wrong signals or crashes.
> +*/
Maybe clarify this? Add some mention that the specific problem is
that user code could set a watchpoint on an address that is also used
in the temporary mm.
Arguably we should not disable *kernel* breakpoints a la perf, but
that seems like quite a minor issue, at least as long as
use_temporary_mm() doesn't get wider use. But a comment that this
also disables perf breakpoints and that this could be undesirable
might be in order as well.
Re: [PATCH v2 05/20] x86/alternative: initializing temporary mm for patching
> On Feb 11, 2019, at 11:07 AM, Andy Lutomirski wrote:
>
> I'm certainly amenable to other solutions, but this one does seem the
> least messy. I looked at my old patch, and it doesn't do what you
> want. I'd suggest you just add a percpu variable like cpu_dr7 and rig
> up some accessors so that it stays up to date. Then you can skip the
> dr7 writes if there are no watchpoints set.
>
> Also, EFI is probably a less interesting example than rare_write.
> With rare_write, especially the dynamically allocated variants that
> people keep coming up with, we'll need a swath of address space fully
> as large as the vmalloc area. and getting *that* right while still
> using the kernel address range might be more of a mess than we really
> want to deal with.
As long as you feel comfortable with this solution, I’m fine with it.
Here is what I have (untested). I prefer to save/restore all the DRs,
because IIRC DR6 indications are updated even if breakpoints are disabled
(in DR7). And anyhow, that is the standard interface.
-- >8 --
From: Nadav Amit
Date: Mon, 11 Feb 2019 03:07:08 -0800
Subject: [PATCH] mm: save DRs when loading temporary mm
Signed-off-by: Nadav Amit
---
arch/x86/include/asm/mmu_context.h | 18 ++
1 file changed, 18 insertions(+)
diff --git a/arch/x86/include/asm/mmu_context.h
b/arch/x86/include/asm/mmu_context.h
index d684b954f3c0..4f92ec3df149 100644
--- a/arch/x86/include/asm/mmu_context.h
+++ b/arch/x86/include/asm/mmu_context.h
@@ -13,6 +13,7 @@
#include
#include
#include
+#include
extern atomic64_t last_mm_ctx_id;
@@ -358,6 +359,7 @@ static inline unsigned long __get_current_cr3_fast(void)
typedef struct {
struct mm_struct *prev;
+ unsigned short bp_enabled : 1;
} temp_mm_state_t;
/*
@@ -380,6 +382,15 @@ static inline temp_mm_state_t use_temporary_mm(struct
mm_struct *mm)
lockdep_assert_irqs_disabled();
state.prev = this_cpu_read(cpu_tlbstate.loaded_mm);
switch_mm_irqs_off(NULL, mm, current);
+
+ /*
+* If breakpoints are enabled, disable them while the temporary mm is
+* used - they do not belong and might cause wrong signals or crashes.
+*/
+ state.bp_enabled = hw_breakpoint_active();
+ if (state.bp_enabled)
+ hw_breakpoint_disable();
+
return state;
}
@@ -387,6 +398,13 @@ static inline void unuse_temporary_mm(temp_mm_state_t prev)
{
lockdep_assert_irqs_disabled();
switch_mm_irqs_off(NULL, prev.prev, current);
+
+ /*
+* Restore the breakpoints if they were disabled before the temporary mm
+* was loaded.
+*/
+ if (prev.bp_enabled)
+ hw_breakpoint_restore();
}
#endif /* _ASM_X86_MMU_CONTEXT_H */
--
2.17.1
Re: [PATCH v2 05/20] x86/alternative: initializing temporary mm for patching
On Mon, Feb 11, 2019 at 10:05 AM Nadav Amit wrote:
>
> > On Feb 10, 2019, at 9:18 PM, Andy Lutomirski wrote:
> >
> >
> >
> > On Feb 10, 2019, at 4:39 PM, Nadav Amit wrote:
> >
> >>> On Jan 28, 2019, at 4:34 PM, Rick Edgecombe
> >>> wrote:
> >>>
> >>> From: Nadav Amit
> >>>
> >>> To prevent improper use of the PTEs that are used for text patching, we
> >>> want to use a temporary mm struct. We initailize it by copying the init
> >>> mm.
> >>>
> >>> The address that will be used for patching is taken from the lower area
> >>> that is usually used for the task memory. Doing so prevents the need to
> >>> frequently synchronize the temporary-mm (e.g., when BPF programs are
> >>> installed), since different PGDs are used for the task memory.
> >>>
> >>> Finally, we randomize the address of the PTEs to harden against exploits
> >>> that use these PTEs.
> >>>
> >>> Cc: Kees Cook
> >>> Cc: Dave Hansen
> >>> Acked-by: Peter Zijlstra (Intel)
> >>> Reviewed-by: Masami Hiramatsu
> >>> Tested-by: Masami Hiramatsu
> >>> Suggested-by: Andy Lutomirski
> >>> Signed-off-by: Nadav Amit
> >>> Signed-off-by: Rick Edgecombe
> >>> ---
> >>> arch/x86/include/asm/pgtable.h | 3 +++
> >>> arch/x86/include/asm/text-patching.h | 2 ++
> >>> arch/x86/kernel/alternative.c| 3 +++
> >>> arch/x86/mm/init_64.c| 36
> >>> init/main.c | 3 +++
> >>> 5 files changed, 47 insertions(+)
> >>>
> >>> diff --git a/arch/x86/include/asm/pgtable.h
> >>> b/arch/x86/include/asm/pgtable.h
> >>> index 40616e805292..e8f630d9a2ed 100644
> >>> --- a/arch/x86/include/asm/pgtable.h
> >>> +++ b/arch/x86/include/asm/pgtable.h
> >>> @@ -1021,6 +1021,9 @@ static inline void __meminit
> >>> init_trampoline_default(void)
> >>> /* Default trampoline pgd value */
> >>> trampoline_pgd_entry = init_top_pgt[pgd_index(__PAGE_OFFSET)];
> >>> }
> >>> +
> >>> +void __init poking_init(void);
> >>> +
> >>> # ifdef CONFIG_RANDOMIZE_MEMORY
> >>> void __meminit init_trampoline(void);
> >>> # else
> >>> diff --git a/arch/x86/include/asm/text-patching.h
> >>> b/arch/x86/include/asm/text-patching.h
> >>> index f8fc8e86cf01..a75eed841eed 100644
> >>> --- a/arch/x86/include/asm/text-patching.h
> >>> +++ b/arch/x86/include/asm/text-patching.h
> >>> @@ -39,5 +39,7 @@ extern void *text_poke_kgdb(void *addr, const void
> >>> *opcode, size_t len);
> >>> extern int poke_int3_handler(struct pt_regs *regs);
> >>> extern void *text_poke_bp(void *addr, const void *opcode, size_t len,
> >>> void *handler);
> >>> extern int after_bootmem;
> >>> +extern __ro_after_init struct mm_struct *poking_mm;
> >>> +extern __ro_after_init unsigned long poking_addr;
> >>>
> >>> #endif /* _ASM_X86_TEXT_PATCHING_H */
> >>> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> >>> index 12fddbc8c55b..ae05fbb50171 100644
> >>> --- a/arch/x86/kernel/alternative.c
> >>> +++ b/arch/x86/kernel/alternative.c
> >>> @@ -678,6 +678,9 @@ void *__init_or_module text_poke_early(void *addr,
> >>> const void *opcode,
> >>> return addr;
> >>> }
> >>>
> >>> +__ro_after_init struct mm_struct *poking_mm;
> >>> +__ro_after_init unsigned long poking_addr;
> >>> +
> >>> static void *__text_poke(void *addr, const void *opcode, size_t len)
> >>> {
> >>> unsigned long flags;
> >>> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> >>> index bccff68e3267..125c8c48aa24 100644
> >>> --- a/arch/x86/mm/init_64.c
> >>> +++ b/arch/x86/mm/init_64.c
> >>> @@ -53,6 +53,7 @@
> >>> #include
> >>> #include
> >>> #include
> >>> +#include
> >>>
> >>> #include "mm_internal.h"
> >>>
> >>> @@ -1383,6 +1384,41 @@ unsigned long memory_block_size_bytes(void)
> >>> return memory_block_size_probed;
> >>> }
> >>>
> >>> +/*
> >>> + * Initialize an mm_struct to be used during poking and a pointer to be
> >>> used
> >>> + * during patching.
> >>> + */
> >>> +void __init poking_init(void)
> >>> +{
> >>> +spinlock_t *ptl;
> >>> +pte_t *ptep;
> >>> +
> >>> +poking_mm = copy_init_mm();
> >>> +BUG_ON(!poking_mm);
> >>> +
> >>> +/*
> >>> + * Randomize the poking address, but make sure that the following
> >>> page
> >>> + * will be mapped at the same PMD. We need 2 pages, so find space
> >>> for 3,
> >>> + * and adjust the address if the PMD ends after the first one.
> >>> + */
> >>> +poking_addr = TASK_UNMAPPED_BASE;
> >>> +if (IS_ENABLED(CONFIG_RANDOMIZE_BASE))
> >>> +poking_addr += (kaslr_get_random_long("Poking") & PAGE_MASK) %
> >>> +(TASK_SIZE - TASK_UNMAPPED_BASE - 3 * PAGE_SIZE);
> >>> +
> >>> +if (((poking_addr + PAGE_SIZE) & ~PMD_MASK) == 0)
> >>> +poking_addr += PAGE_SIZE;
> >>
> >> Further thinking about it, I think that allocating the virtual address for
> >> poking from user address-range is problematic. The user can set watchpoints
> >> on different addresses, cause some static-keys to be enabled/disabled, and
> >> monitor the
Re: [PATCH v2 05/20] x86/alternative: initializing temporary mm for patching
> On Feb 10, 2019, at 9:18 PM, Andy Lutomirski wrote:
>
>
>
> On Feb 10, 2019, at 4:39 PM, Nadav Amit wrote:
>
>>> On Jan 28, 2019, at 4:34 PM, Rick Edgecombe
>>> wrote:
>>>
>>> From: Nadav Amit
>>>
>>> To prevent improper use of the PTEs that are used for text patching, we
>>> want to use a temporary mm struct. We initailize it by copying the init
>>> mm.
>>>
>>> The address that will be used for patching is taken from the lower area
>>> that is usually used for the task memory. Doing so prevents the need to
>>> frequently synchronize the temporary-mm (e.g., when BPF programs are
>>> installed), since different PGDs are used for the task memory.
>>>
>>> Finally, we randomize the address of the PTEs to harden against exploits
>>> that use these PTEs.
>>>
>>> Cc: Kees Cook
>>> Cc: Dave Hansen
>>> Acked-by: Peter Zijlstra (Intel)
>>> Reviewed-by: Masami Hiramatsu
>>> Tested-by: Masami Hiramatsu
>>> Suggested-by: Andy Lutomirski
>>> Signed-off-by: Nadav Amit
>>> Signed-off-by: Rick Edgecombe
>>> ---
>>> arch/x86/include/asm/pgtable.h | 3 +++
>>> arch/x86/include/asm/text-patching.h | 2 ++
>>> arch/x86/kernel/alternative.c| 3 +++
>>> arch/x86/mm/init_64.c| 36
>>> init/main.c | 3 +++
>>> 5 files changed, 47 insertions(+)
>>>
>>> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
>>> index 40616e805292..e8f630d9a2ed 100644
>>> --- a/arch/x86/include/asm/pgtable.h
>>> +++ b/arch/x86/include/asm/pgtable.h
>>> @@ -1021,6 +1021,9 @@ static inline void __meminit
>>> init_trampoline_default(void)
>>> /* Default trampoline pgd value */
>>> trampoline_pgd_entry = init_top_pgt[pgd_index(__PAGE_OFFSET)];
>>> }
>>> +
>>> +void __init poking_init(void);
>>> +
>>> # ifdef CONFIG_RANDOMIZE_MEMORY
>>> void __meminit init_trampoline(void);
>>> # else
>>> diff --git a/arch/x86/include/asm/text-patching.h
>>> b/arch/x86/include/asm/text-patching.h
>>> index f8fc8e86cf01..a75eed841eed 100644
>>> --- a/arch/x86/include/asm/text-patching.h
>>> +++ b/arch/x86/include/asm/text-patching.h
>>> @@ -39,5 +39,7 @@ extern void *text_poke_kgdb(void *addr, const void
>>> *opcode, size_t len);
>>> extern int poke_int3_handler(struct pt_regs *regs);
>>> extern void *text_poke_bp(void *addr, const void *opcode, size_t len, void
>>> *handler);
>>> extern int after_bootmem;
>>> +extern __ro_after_init struct mm_struct *poking_mm;
>>> +extern __ro_after_init unsigned long poking_addr;
>>>
>>> #endif /* _ASM_X86_TEXT_PATCHING_H */
>>> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
>>> index 12fddbc8c55b..ae05fbb50171 100644
>>> --- a/arch/x86/kernel/alternative.c
>>> +++ b/arch/x86/kernel/alternative.c
>>> @@ -678,6 +678,9 @@ void *__init_or_module text_poke_early(void *addr,
>>> const void *opcode,
>>> return addr;
>>> }
>>>
>>> +__ro_after_init struct mm_struct *poking_mm;
>>> +__ro_after_init unsigned long poking_addr;
>>> +
>>> static void *__text_poke(void *addr, const void *opcode, size_t len)
>>> {
>>> unsigned long flags;
>>> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
>>> index bccff68e3267..125c8c48aa24 100644
>>> --- a/arch/x86/mm/init_64.c
>>> +++ b/arch/x86/mm/init_64.c
>>> @@ -53,6 +53,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>>
>>> #include "mm_internal.h"
>>>
>>> @@ -1383,6 +1384,41 @@ unsigned long memory_block_size_bytes(void)
>>> return memory_block_size_probed;
>>> }
>>>
>>> +/*
>>> + * Initialize an mm_struct to be used during poking and a pointer to be
>>> used
>>> + * during patching.
>>> + */
>>> +void __init poking_init(void)
>>> +{
>>> +spinlock_t *ptl;
>>> +pte_t *ptep;
>>> +
>>> +poking_mm = copy_init_mm();
>>> +BUG_ON(!poking_mm);
>>> +
>>> +/*
>>> + * Randomize the poking address, but make sure that the following page
>>> + * will be mapped at the same PMD. We need 2 pages, so find space for
>>> 3,
>>> + * and adjust the address if the PMD ends after the first one.
>>> + */
>>> +poking_addr = TASK_UNMAPPED_BASE;
>>> +if (IS_ENABLED(CONFIG_RANDOMIZE_BASE))
>>> +poking_addr += (kaslr_get_random_long("Poking") & PAGE_MASK) %
>>> +(TASK_SIZE - TASK_UNMAPPED_BASE - 3 * PAGE_SIZE);
>>> +
>>> +if (((poking_addr + PAGE_SIZE) & ~PMD_MASK) == 0)
>>> +poking_addr += PAGE_SIZE;
>>
>> Further thinking about it, I think that allocating the virtual address for
>> poking from user address-range is problematic. The user can set watchpoints
>> on different addresses, cause some static-keys to be enabled/disabled, and
>> monitor the signals to derandomize the poking address.
>
> Hmm, I hadn’t thought about watchpoints. I’m not sure how much we care
> about possible derandomization like this, but we certainly don’t want to
> send signals or otherwise malfunction.
>
>> Andy, I think you were pushing this change. Can I go back to use a
Re: [PATCH v2 05/20] x86/alternative: initializing temporary mm for patching
On Feb 10, 2019, at 4:39 PM, Nadav Amit wrote:
>> On Jan 28, 2019, at 4:34 PM, Rick Edgecombe
>> wrote:
>>
>> From: Nadav Amit
>>
>> To prevent improper use of the PTEs that are used for text patching, we
>> want to use a temporary mm struct. We initailize it by copying the init
>> mm.
>>
>> The address that will be used for patching is taken from the lower area
>> that is usually used for the task memory. Doing so prevents the need to
>> frequently synchronize the temporary-mm (e.g., when BPF programs are
>> installed), since different PGDs are used for the task memory.
>>
>> Finally, we randomize the address of the PTEs to harden against exploits
>> that use these PTEs.
>>
>> Cc: Kees Cook
>> Cc: Dave Hansen
>> Acked-by: Peter Zijlstra (Intel)
>> Reviewed-by: Masami Hiramatsu
>> Tested-by: Masami Hiramatsu
>> Suggested-by: Andy Lutomirski
>> Signed-off-by: Nadav Amit
>> Signed-off-by: Rick Edgecombe
>> ---
>> arch/x86/include/asm/pgtable.h | 3 +++
>> arch/x86/include/asm/text-patching.h | 2 ++
>> arch/x86/kernel/alternative.c| 3 +++
>> arch/x86/mm/init_64.c| 36
>> init/main.c | 3 +++
>> 5 files changed, 47 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
>> index 40616e805292..e8f630d9a2ed 100644
>> --- a/arch/x86/include/asm/pgtable.h
>> +++ b/arch/x86/include/asm/pgtable.h
>> @@ -1021,6 +1021,9 @@ static inline void __meminit
>> init_trampoline_default(void)
>>/* Default trampoline pgd value */
>>trampoline_pgd_entry = init_top_pgt[pgd_index(__PAGE_OFFSET)];
>> }
>> +
>> +void __init poking_init(void);
>> +
>> # ifdef CONFIG_RANDOMIZE_MEMORY
>> void __meminit init_trampoline(void);
>> # else
>> diff --git a/arch/x86/include/asm/text-patching.h
>> b/arch/x86/include/asm/text-patching.h
>> index f8fc8e86cf01..a75eed841eed 100644
>> --- a/arch/x86/include/asm/text-patching.h
>> +++ b/arch/x86/include/asm/text-patching.h
>> @@ -39,5 +39,7 @@ extern void *text_poke_kgdb(void *addr, const void
>> *opcode, size_t len);
>> extern int poke_int3_handler(struct pt_regs *regs);
>> extern void *text_poke_bp(void *addr, const void *opcode, size_t len, void
>> *handler);
>> extern int after_bootmem;
>> +extern __ro_after_init struct mm_struct *poking_mm;
>> +extern __ro_after_init unsigned long poking_addr;
>>
>> #endif /* _ASM_X86_TEXT_PATCHING_H */
>> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
>> index 12fddbc8c55b..ae05fbb50171 100644
>> --- a/arch/x86/kernel/alternative.c
>> +++ b/arch/x86/kernel/alternative.c
>> @@ -678,6 +678,9 @@ void *__init_or_module text_poke_early(void *addr, const
>> void *opcode,
>>return addr;
>> }
>>
>> +__ro_after_init struct mm_struct *poking_mm;
>> +__ro_after_init unsigned long poking_addr;
>> +
>> static void *__text_poke(void *addr, const void *opcode, size_t len)
>> {
>>unsigned long flags;
>> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
>> index bccff68e3267..125c8c48aa24 100644
>> --- a/arch/x86/mm/init_64.c
>> +++ b/arch/x86/mm/init_64.c
>> @@ -53,6 +53,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include "mm_internal.h"
>>
>> @@ -1383,6 +1384,41 @@ unsigned long memory_block_size_bytes(void)
>>return memory_block_size_probed;
>> }
>>
>> +/*
>> + * Initialize an mm_struct to be used during poking and a pointer to be used
>> + * during patching.
>> + */
>> +void __init poking_init(void)
>> +{
>> +spinlock_t *ptl;
>> +pte_t *ptep;
>> +
>> +poking_mm = copy_init_mm();
>> +BUG_ON(!poking_mm);
>> +
>> +/*
>> + * Randomize the poking address, but make sure that the following page
>> + * will be mapped at the same PMD. We need 2 pages, so find space for 3,
>> + * and adjust the address if the PMD ends after the first one.
>> + */
>> +poking_addr = TASK_UNMAPPED_BASE;
>> +if (IS_ENABLED(CONFIG_RANDOMIZE_BASE))
>> +poking_addr += (kaslr_get_random_long("Poking") & PAGE_MASK) %
>> +(TASK_SIZE - TASK_UNMAPPED_BASE - 3 * PAGE_SIZE);
>> +
>> +if (((poking_addr + PAGE_SIZE) & ~PMD_MASK) == 0)
>> +poking_addr += PAGE_SIZE;
>
> Further thinking about it, I think that allocating the virtual address for
> poking from user address-range is problematic. The user can set watchpoints
> on different addresses, cause some static-keys to be enabled/disabled, and
> monitor the signals to derandomize the poking address.
>
Hmm, I hadn’t thought about watchpoints. I’m not sure how much we care about
possible derandomization like this, but we certainly don’t want to send signals
or otherwise malfunction.
> Andy, I think you were pushing this change. Can I go back to use a vmalloc’d
> address instead, or do you have a better solution?
Hmm. If we use a vmalloc address, we have to make sure it’s not actually
allocated. I suppose we could allocate one once at boot and use
Re: [PATCH v2 05/20] x86/alternative: initializing temporary mm for patching
> On Jan 28, 2019, at 4:34 PM, Rick Edgecombe
> wrote:
>
> From: Nadav Amit
>
> To prevent improper use of the PTEs that are used for text patching, we
> want to use a temporary mm struct. We initailize it by copying the init
> mm.
>
> The address that will be used for patching is taken from the lower area
> that is usually used for the task memory. Doing so prevents the need to
> frequently synchronize the temporary-mm (e.g., when BPF programs are
> installed), since different PGDs are used for the task memory.
>
> Finally, we randomize the address of the PTEs to harden against exploits
> that use these PTEs.
>
> Cc: Kees Cook
> Cc: Dave Hansen
> Acked-by: Peter Zijlstra (Intel)
> Reviewed-by: Masami Hiramatsu
> Tested-by: Masami Hiramatsu
> Suggested-by: Andy Lutomirski
> Signed-off-by: Nadav Amit
> Signed-off-by: Rick Edgecombe
> ---
> arch/x86/include/asm/pgtable.h | 3 +++
> arch/x86/include/asm/text-patching.h | 2 ++
> arch/x86/kernel/alternative.c| 3 +++
> arch/x86/mm/init_64.c| 36
> init/main.c | 3 +++
> 5 files changed, 47 insertions(+)
>
> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> index 40616e805292..e8f630d9a2ed 100644
> --- a/arch/x86/include/asm/pgtable.h
> +++ b/arch/x86/include/asm/pgtable.h
> @@ -1021,6 +1021,9 @@ static inline void __meminit
> init_trampoline_default(void)
> /* Default trampoline pgd value */
> trampoline_pgd_entry = init_top_pgt[pgd_index(__PAGE_OFFSET)];
> }
> +
> +void __init poking_init(void);
> +
> # ifdef CONFIG_RANDOMIZE_MEMORY
> void __meminit init_trampoline(void);
> # else
> diff --git a/arch/x86/include/asm/text-patching.h
> b/arch/x86/include/asm/text-patching.h
> index f8fc8e86cf01..a75eed841eed 100644
> --- a/arch/x86/include/asm/text-patching.h
> +++ b/arch/x86/include/asm/text-patching.h
> @@ -39,5 +39,7 @@ extern void *text_poke_kgdb(void *addr, const void *opcode,
> size_t len);
> extern int poke_int3_handler(struct pt_regs *regs);
> extern void *text_poke_bp(void *addr, const void *opcode, size_t len, void
> *handler);
> extern int after_bootmem;
> +extern __ro_after_init struct mm_struct *poking_mm;
> +extern __ro_after_init unsigned long poking_addr;
>
> #endif /* _ASM_X86_TEXT_PATCHING_H */
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 12fddbc8c55b..ae05fbb50171 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -678,6 +678,9 @@ void *__init_or_module text_poke_early(void *addr, const
> void *opcode,
> return addr;
> }
>
> +__ro_after_init struct mm_struct *poking_mm;
> +__ro_after_init unsigned long poking_addr;
> +
> static void *__text_poke(void *addr, const void *opcode, size_t len)
> {
> unsigned long flags;
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index bccff68e3267..125c8c48aa24 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -53,6 +53,7 @@
> #include
> #include
> #include
> +#include
>
> #include "mm_internal.h"
>
> @@ -1383,6 +1384,41 @@ unsigned long memory_block_size_bytes(void)
> return memory_block_size_probed;
> }
>
> +/*
> + * Initialize an mm_struct to be used during poking and a pointer to be used
> + * during patching.
> + */
> +void __init poking_init(void)
> +{
> + spinlock_t *ptl;
> + pte_t *ptep;
> +
> + poking_mm = copy_init_mm();
> + BUG_ON(!poking_mm);
> +
> + /*
> + * Randomize the poking address, but make sure that the following page
> + * will be mapped at the same PMD. We need 2 pages, so find space for 3,
> + * and adjust the address if the PMD ends after the first one.
> + */
> + poking_addr = TASK_UNMAPPED_BASE;
> + if (IS_ENABLED(CONFIG_RANDOMIZE_BASE))
> + poking_addr += (kaslr_get_random_long("Poking") & PAGE_MASK) %
> + (TASK_SIZE - TASK_UNMAPPED_BASE - 3 * PAGE_SIZE);
> +
> + if (((poking_addr + PAGE_SIZE) & ~PMD_MASK) == 0)
> + poking_addr += PAGE_SIZE;
Further thinking about it, I think that allocating the virtual address for
poking from user address-range is problematic. The user can set watchpoints
on different addresses, cause some static-keys to be enabled/disabled, and
monitor the signals to derandomize the poking address.
Andy, I think you were pushing this change. Can I go back to use a vmalloc’d
address instead, or do you have a better solution? I prefer not to
save/restore DR7, of course.
Re: [PATCH v2 05/20] x86/alternative: initializing temporary mm for patching
Just nitpicks: Subject: [PATCH v2 05/20] x86/alternative: initializing temporary mm for patching s/initailizing/Initialize/ On Mon, Jan 28, 2019 at 04:34:07PM -0800, Rick Edgecombe wrote: > From: Nadav Amit > > To prevent improper use of the PTEs that are used for text patching, we > want to use a temporary mm struct. We initailize it by copying the init Please remove the "we" from commit messages and use impartial, passive formulations. Thx. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
