Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[Pratyush Anand]

Hi Will,

Thanks for the reply.

On 21/03/2016:02:52:43 PM, Will Deacon wrote:
> On Fri, Mar 18, 2016 at 06:59:02PM +0530, Pratyush Anand wrote:
> > On 17/03/2016:01:27:26 PM, Pratyush Anand wrote:
> > > @David: This patch was added in v9 and fixup_exception() had been dropped 
> > > in v9.
> > > Since, dropping of fixup_exception() also caused to fail some systemtap 
> > > test
> > > cases, so it was added back in v10. I wonder if we really need this patch.
> > > May be you can try to run related test case by dropping this patch. 
> > 
> > Had a closer look to the code, and noticed that fixup_exception() does not 
> > have
> > any role in handling of page fault of copy_to_user(). Then, why do we have 
> > the
> > problem.
> > Probably, I can see why does not it work. So, when we are single stepping an
> > instruction and page fault occurs, we will come to el1_da in entry.S. Here, 
> > we
> > do enable_dbg. As soon as we will do this, we will start receiving single 
> > step
> > exception after each instruction (not sure, probably for each alternate
> > instruction). Since, there will not be any matching single step handler for
> > these instructions, so we will see warning "Unexpected kernel single-step
> > exception at EL1". 
> > 
> > So, I think, we should 
> > 
> > (1) may be do not enable debug for el1_da, or
> > (2) enable_dbg only when single stepping is not enabled, or
> > (3) or disable single stepping during el1_da execution.
> > 
> > (1) will solve the issue for sure, but not sure if it could be the best 
> > choice.
> > 
> > Will, what do you suggest?
> 
> Leaving debug exceptions disabled isn't something I'm keen on at all,
> because it leads to blackspots in kernel debugging that I don't think
> should be enforced by the low-level debug machinery. My preference is
> for the higher-level debugger code (e.g. kprobes, kdgb) to ignore the
> events that it's not interested in.

I think this is what the current implementation is, so in the given situation
higher-level debugger code ignore the single step exceptions events, which they
are not expecting.
Here, execution of single stepped instruction is causing to raise another new
exception, say data abort. Now, as soon as we enable debug exceptions while
handling this data abort we will start getting single step exceptions for all
the executed instruction of data abort handler. None of the "higher-level
debugger code" is interested in those events and so they ignore them. We keep on
getting "Unexpected kernel single-step exception at EL1" until all the
instructions for data abort handler are executed.

> 
> It's also very easy to lose track of the debug state if you run preemptible
> code at EL1 with debug exceptions disabled, because kernel debugging is
> per-cpu rather than per-task.

OK.Thanks for this clarification. So, one of the way could be to set a per
cpu variable by higher level debugger code, and then check them in kernel_entry
and kernel_exit and accordingly disable/enable only single stepping. Do you
think, it would be good idea to do that?
If yes, then would adding a new u64 variable say "flags" in struct pt_regs be
acceptable? 

~Pratyush

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[Pratyush Anand]

Hi Will,

Thanks for the reply.

On 21/03/2016:02:52:43 PM, Will Deacon wrote:
> On Fri, Mar 18, 2016 at 06:59:02PM +0530, Pratyush Anand wrote:
> > On 17/03/2016:01:27:26 PM, Pratyush Anand wrote:
> > > @David: This patch was added in v9 and fixup_exception() had been dropped 
> > > in v9.
> > > Since, dropping of fixup_exception() also caused to fail some systemtap 
> > > test
> > > cases, so it was added back in v10. I wonder if we really need this patch.
> > > May be you can try to run related test case by dropping this patch. 
> > 
> > Had a closer look to the code, and noticed that fixup_exception() does not 
> > have
> > any role in handling of page fault of copy_to_user(). Then, why do we have 
> > the
> > problem.
> > Probably, I can see why does not it work. So, when we are single stepping an
> > instruction and page fault occurs, we will come to el1_da in entry.S. Here, 
> > we
> > do enable_dbg. As soon as we will do this, we will start receiving single 
> > step
> > exception after each instruction (not sure, probably for each alternate
> > instruction). Since, there will not be any matching single step handler for
> > these instructions, so we will see warning "Unexpected kernel single-step
> > exception at EL1". 
> > 
> > So, I think, we should 
> > 
> > (1) may be do not enable debug for el1_da, or
> > (2) enable_dbg only when single stepping is not enabled, or
> > (3) or disable single stepping during el1_da execution.
> > 
> > (1) will solve the issue for sure, but not sure if it could be the best 
> > choice.
> > 
> > Will, what do you suggest?
> 
> Leaving debug exceptions disabled isn't something I'm keen on at all,
> because it leads to blackspots in kernel debugging that I don't think
> should be enforced by the low-level debug machinery. My preference is
> for the higher-level debugger code (e.g. kprobes, kdgb) to ignore the
> events that it's not interested in.

I think this is what the current implementation is, so in the given situation
higher-level debugger code ignore the single step exceptions events, which they
are not expecting.
Here, execution of single stepped instruction is causing to raise another new
exception, say data abort. Now, as soon as we enable debug exceptions while
handling this data abort we will start getting single step exceptions for all
the executed instruction of data abort handler. None of the "higher-level
debugger code" is interested in those events and so they ignore them. We keep on
getting "Unexpected kernel single-step exception at EL1" until all the
instructions for data abort handler are executed.

> 
> It's also very easy to lose track of the debug state if you run preemptible
> code at EL1 with debug exceptions disabled, because kernel debugging is
> per-cpu rather than per-task.

OK.Thanks for this clarification. So, one of the way could be to set a per
cpu variable by higher level debugger code, and then check them in kernel_entry
and kernel_exit and accordingly disable/enable only single stepping. Do you
think, it would be good idea to do that?
If yes, then would adding a new u64 variable say "flags" in struct pt_regs be
acceptable? 

~Pratyush

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[Will Deacon]

On Fri, Mar 18, 2016 at 06:59:02PM +0530, Pratyush Anand wrote:
> On 17/03/2016:01:27:26 PM, Pratyush Anand wrote:
> > @David: This patch was added in v9 and fixup_exception() had been dropped 
> > in v9.
> > Since, dropping of fixup_exception() also caused to fail some systemtap test
> > cases, so it was added back in v10. I wonder if we really need this patch.
> > May be you can try to run related test case by dropping this patch. 
> 
> Had a closer look to the code, and noticed that fixup_exception() does not 
> have
> any role in handling of page fault of copy_to_user(). Then, why do we have the
> problem.
> Probably, I can see why does not it work. So, when we are single stepping an
> instruction and page fault occurs, we will come to el1_da in entry.S. Here, we
> do enable_dbg. As soon as we will do this, we will start receiving single step
> exception after each instruction (not sure, probably for each alternate
> instruction). Since, there will not be any matching single step handler for
> these instructions, so we will see warning "Unexpected kernel single-step
> exception at EL1". 
> 
> So, I think, we should 
> 
> (1) may be do not enable debug for el1_da, or
> (2) enable_dbg only when single stepping is not enabled, or
> (3) or disable single stepping during el1_da execution.
> 
> (1) will solve the issue for sure, but not sure if it could be the best 
> choice.
> 
> Will, what do you suggest?

Leaving debug exceptions disabled isn't something I'm keen on at all,
because it leads to blackspots in kernel debugging that I don't think
should be enforced by the low-level debug machinery. My preference is
for the higher-level debugger code (e.g. kprobes, kdgb) to ignore the
events that it's not interested in.

It's also very easy to lose track of the debug state if you run preemptible
code at EL1 with debug exceptions disabled, because kernel debugging is
per-cpu rather than per-task.

Will

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[Will Deacon]

On Fri, Mar 18, 2016 at 06:59:02PM +0530, Pratyush Anand wrote:
> On 17/03/2016:01:27:26 PM, Pratyush Anand wrote:
> > @David: This patch was added in v9 and fixup_exception() had been dropped 
> > in v9.
> > Since, dropping of fixup_exception() also caused to fail some systemtap test
> > cases, so it was added back in v10. I wonder if we really need this patch.
> > May be you can try to run related test case by dropping this patch. 
> 
> Had a closer look to the code, and noticed that fixup_exception() does not 
> have
> any role in handling of page fault of copy_to_user(). Then, why do we have the
> problem.
> Probably, I can see why does not it work. So, when we are single stepping an
> instruction and page fault occurs, we will come to el1_da in entry.S. Here, we
> do enable_dbg. As soon as we will do this, we will start receiving single step
> exception after each instruction (not sure, probably for each alternate
> instruction). Since, there will not be any matching single step handler for
> these instructions, so we will see warning "Unexpected kernel single-step
> exception at EL1". 
> 
> So, I think, we should 
> 
> (1) may be do not enable debug for el1_da, or
> (2) enable_dbg only when single stepping is not enabled, or
> (3) or disable single stepping during el1_da execution.
> 
> (1) will solve the issue for sure, but not sure if it could be the best 
> choice.
> 
> Will, what do you suggest?

Leaving debug exceptions disabled isn't something I'm keen on at all,
because it leads to blackspots in kernel debugging that I don't think
should be enforced by the low-level debug machinery. My preference is
for the higher-level debugger code (e.g. kprobes, kdgb) to ignore the
events that it's not interested in.

It's also very easy to lose track of the debug state if you run preemptible
code at EL1 with debug exceptions disabled, because kernel debugging is
per-cpu rather than per-task.

Will

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[Pratyush Anand]

Hi James,

On 18/03/2016:06:12:20 PM, James Morse wrote:
> Hi Pratyush,
> 
> On 18/03/16 14:43, Pratyush Anand wrote:
> > On 18/03/2016:02:02:49 PM, James Morse wrote:
> >> In kernel/entry.S when entered from EL0 we test for TIF_SINGLESTEP in the
> >> thread_info flags, and use disable_step_tsk/enable_step_tsk to 
> >> save/restore the
> >> single-step state.
> >>
> >> Could we do this regardless of which EL we came from?
> > 
> > Thanks for another idea. I think, we can not do this as it is, because
> > TIF_SINGLESTEP will not be set for kprobe events.
> 
> Hmmm, I see kernel_enable_single_step() doesn't set it, but setup_singlestep()
> in patch 5 could...
> 
> There is probably a good reason its never set for a kernel thread, I will 
> have a
> look at where else it is used.
> 
> 
> > But, we can introduce a
> > variant disable_step_kernel and enable_step_kernel, which can be called in
> > el1_da. 
> 
> What about sp/pc misalignment, or undefined instructions?
> Or worse... an irq occurs during your el1_da call (el1_da may re-enable irqs).
> el1_irq doesn't know you were careful not to unmask debug exceptions, it 
> blindly
> turns them back on.
> 
> The problem is the 'single step me' bit is still set, save/restoring it will
> save us having to consider every interaction, (and then missing some!).
> 
> It would also mean you don't have to disable interrupts while single stepping 
> in
> patch 5 (comment above kprobes_save_local_irqflag()).

I see.
kernel_enable_single_step() is called from watchpoint and kgdb handler. It seems
to me that, similar issue may arise there as well. So, it would be a good idea
to set TIF_SINGLESTEP in kernel_enable_single_step() and clear in
kernel_disable_single_step().

Meanwhile, I prepared a test case to reproduce the issue without this patch.
Instrumented a kprobe at an instruction of __copy_to_user() which stores in user
space memory. I can see a sea of messages "Unexpected kernel single-step
exception at EL1" within few seconds.  While with patch[1] applied, I do not see
any such messages. 

May be I can send [1] as RFC and seek feedback.

~Pratyush

[1] 
https://github.com/pratyushanand/linux/commit/7623c8099ac22eaa00e7e0f52430f7a4bd154652

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[Pratyush Anand]

Hi James,

On 18/03/2016:06:12:20 PM, James Morse wrote:
> Hi Pratyush,
> 
> On 18/03/16 14:43, Pratyush Anand wrote:
> > On 18/03/2016:02:02:49 PM, James Morse wrote:
> >> In kernel/entry.S when entered from EL0 we test for TIF_SINGLESTEP in the
> >> thread_info flags, and use disable_step_tsk/enable_step_tsk to 
> >> save/restore the
> >> single-step state.
> >>
> >> Could we do this regardless of which EL we came from?
> > 
> > Thanks for another idea. I think, we can not do this as it is, because
> > TIF_SINGLESTEP will not be set for kprobe events.
> 
> Hmmm, I see kernel_enable_single_step() doesn't set it, but setup_singlestep()
> in patch 5 could...
> 
> There is probably a good reason its never set for a kernel thread, I will 
> have a
> look at where else it is used.
> 
> 
> > But, we can introduce a
> > variant disable_step_kernel and enable_step_kernel, which can be called in
> > el1_da. 
> 
> What about sp/pc misalignment, or undefined instructions?
> Or worse... an irq occurs during your el1_da call (el1_da may re-enable irqs).
> el1_irq doesn't know you were careful not to unmask debug exceptions, it 
> blindly
> turns them back on.
> 
> The problem is the 'single step me' bit is still set, save/restoring it will
> save us having to consider every interaction, (and then missing some!).
> 
> It would also mean you don't have to disable interrupts while single stepping 
> in
> patch 5 (comment above kprobes_save_local_irqflag()).

I see.
kernel_enable_single_step() is called from watchpoint and kgdb handler. It seems
to me that, similar issue may arise there as well. So, it would be a good idea
to set TIF_SINGLESTEP in kernel_enable_single_step() and clear in
kernel_disable_single_step().

Meanwhile, I prepared a test case to reproduce the issue without this patch.
Instrumented a kprobe at an instruction of __copy_to_user() which stores in user
space memory. I can see a sea of messages "Unexpected kernel single-step
exception at EL1" within few seconds.  While with patch[1] applied, I do not see
any such messages. 

May be I can send [1] as RFC and seek feedback.

~Pratyush

[1] 
https://github.com/pratyushanand/linux/commit/7623c8099ac22eaa00e7e0f52430f7a4bd154652

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[James Morse]

Hi Pratyush,

On 18/03/16 13:29, Pratyush Anand wrote:
> Probably, I can see why does not it work. So, when we are single stepping an
> instruction and page fault occurs, we will come to el1_da in entry.S. Here, we
> do enable_dbg. As soon as we will do this, we will start receiving single step
> exception after each instruction (not sure, probably for each alternate
> instruction). Since, there will not be any matching single step handler for
> these instructions, so we will see warning "Unexpected kernel single-step
> exception at EL1". 
> 
> So, I think, we should 
> 
> (1) may be do not enable debug for el1_da, or
> (2) enable_dbg only when single stepping is not enabled, or
> (3) or disable single stepping during el1_da execution.
> 
> (1) will solve the issue for sure, but not sure if it could be the best 
> choice.

A variation on (3):

In kernel/entry.S when entered from EL0 we test for TIF_SINGLESTEP in the
thread_info flags, and use disable_step_tsk/enable_step_tsk to save/restore the
single-step state.

Could we do this regardless of which EL we came from?


Thanks,

James

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[James Morse]

Hi Pratyush,

On 18/03/16 13:29, Pratyush Anand wrote:
> Probably, I can see why does not it work. So, when we are single stepping an
> instruction and page fault occurs, we will come to el1_da in entry.S. Here, we
> do enable_dbg. As soon as we will do this, we will start receiving single step
> exception after each instruction (not sure, probably for each alternate
> instruction). Since, there will not be any matching single step handler for
> these instructions, so we will see warning "Unexpected kernel single-step
> exception at EL1". 
> 
> So, I think, we should 
> 
> (1) may be do not enable debug for el1_da, or
> (2) enable_dbg only when single stepping is not enabled, or
> (3) or disable single stepping during el1_da execution.
> 
> (1) will solve the issue for sure, but not sure if it could be the best 
> choice.

A variation on (3):

In kernel/entry.S when entered from EL0 we test for TIF_SINGLESTEP in the
thread_info flags, and use disable_step_tsk/enable_step_tsk to save/restore the
single-step state.

Could we do this regardless of which EL we came from?


Thanks,

James

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[Pratyush Anand]

Hi James,

On 16/03/2016:10:27:22 AM, James Morse wrote:
> Hi Pratyush,
> 
> On 16/03/16 05:43, Pratyush Anand wrote:
> > On 15/03/2016:06:47:52 PM, James Morse wrote:
> >> If I understand this correctly -  you can't kprobe these ldr/str 
> >> instructions
> >> as the fault handler wouldn't find kprobe's out-of line version of the
> >> instruction in the exception table... but why only these two functions? 
> >> (for
> >> library functions, we also have clear_user() and copy_in_user()...)
> >
> > May be not clear_user() because those are inlined, but may be 
> > __clear_user().
> 
> You're right - the other library functions in that same directory is what I 
> meant..
> 
> >> Is it feasible to search the exception table at runtime instead? If an
> >> address-to-be-kprobed appears in the list, we know it could generate 
> >> exceptions,
> >> so we should report that we can't probe this address. That would catch all 
> >> of
> >> the library functions, all the places uaccess.h was inlined, and anything 
> >> new
> >> that gets invented in the future.
> > 
> > Sorry, probably I could not get it. How can an inlined addresses range be 
> > placed
> > in exception table or any other code area.
> 
> Ah, not a section or code area, sorry I wasn't clear:
> 
> When a fault happens in the kernel, the fault handler
> (/arch/arm64/mm/fault.c:do_page_fault()) calls 
> search_exception_tables(regs->pc)
> to see if the faulting address has a 'fixup' registered. If it does, the fixup
> causes -EFAULT to be returned, if not it ends up in die().
> 
> The horrible block of assembler in
> arch/arm64/include/asm/uaccess.h:__get_user_asm() adds the address of the
> instruction that is allowed to fault to the __ex_table section:
> > .section __ex_table,"a"
> > .align  3
> > .quad   1b, 3b
> > .previous
> 
> Here 1b is the address of the instruction that can fault, and 3b is the fixup
> that moves -EFAULT into the return value.
> 
> This works for get_user() and friends which are inlined all over the kernel. 
> It
> even works for modules, as there is an exception table for each module which 
> is
> searched by kernel/module.c:search_module_extables().
> 
> This list of addresses that can fault already exists, there is even an API
> function to check for a given address. Grabbing the nearest vmlinux, there are
> ~1300 entries in the __ex_table section, this patch blacklists two of them,
> using search_exception_tables() obviously blacklists them all.

Thanks a lot for explaining it. Got it now. So agreeing to your idea. But

> 
> 
> I've had a quick look at x86 and sparc, it looks like they allowed probed
> instructions to fault, 
> do_page_fault()->kprobes_fault()->kprobe_fault_handler()
> - which uses the original probed address with search_exception_tables() to 
> find
> and run the fixup. I doubt this is needed in an initial version of kprobes,
> (maybe its later in this series - I haven't read all the way through it yet).

Hu..We do have fixup_exception() in arm64 kprobe_fault_handler(). So, it
should have worked, without this patch.

@David: This patch was added in v9 and fixup_exception() had been dropped in v9.
Since, dropping of fixup_exception() also caused to fail some systemtap test
cases, so it was added back in v10. I wonder if we really need this patch.
May be you can try to run related test case by dropping this patch. 

Thanks James for bringing this out.

~Pratyush

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[Pratyush Anand]

Hi James,

On 16/03/2016:10:27:22 AM, James Morse wrote:
> Hi Pratyush,
> 
> On 16/03/16 05:43, Pratyush Anand wrote:
> > On 15/03/2016:06:47:52 PM, James Morse wrote:
> >> If I understand this correctly -  you can't kprobe these ldr/str 
> >> instructions
> >> as the fault handler wouldn't find kprobe's out-of line version of the
> >> instruction in the exception table... but why only these two functions? 
> >> (for
> >> library functions, we also have clear_user() and copy_in_user()...)
> >
> > May be not clear_user() because those are inlined, but may be 
> > __clear_user().
> 
> You're right - the other library functions in that same directory is what I 
> meant..
> 
> >> Is it feasible to search the exception table at runtime instead? If an
> >> address-to-be-kprobed appears in the list, we know it could generate 
> >> exceptions,
> >> so we should report that we can't probe this address. That would catch all 
> >> of
> >> the library functions, all the places uaccess.h was inlined, and anything 
> >> new
> >> that gets invented in the future.
> > 
> > Sorry, probably I could not get it. How can an inlined addresses range be 
> > placed
> > in exception table or any other code area.
> 
> Ah, not a section or code area, sorry I wasn't clear:
> 
> When a fault happens in the kernel, the fault handler
> (/arch/arm64/mm/fault.c:do_page_fault()) calls 
> search_exception_tables(regs->pc)
> to see if the faulting address has a 'fixup' registered. If it does, the fixup
> causes -EFAULT to be returned, if not it ends up in die().
> 
> The horrible block of assembler in
> arch/arm64/include/asm/uaccess.h:__get_user_asm() adds the address of the
> instruction that is allowed to fault to the __ex_table section:
> > .section __ex_table,"a"
> > .align  3
> > .quad   1b, 3b
> > .previous
> 
> Here 1b is the address of the instruction that can fault, and 3b is the fixup
> that moves -EFAULT into the return value.
> 
> This works for get_user() and friends which are inlined all over the kernel. 
> It
> even works for modules, as there is an exception table for each module which 
> is
> searched by kernel/module.c:search_module_extables().
> 
> This list of addresses that can fault already exists, there is even an API
> function to check for a given address. Grabbing the nearest vmlinux, there are
> ~1300 entries in the __ex_table section, this patch blacklists two of them,
> using search_exception_tables() obviously blacklists them all.

Thanks a lot for explaining it. Got it now. So agreeing to your idea. But

> 
> 
> I've had a quick look at x86 and sparc, it looks like they allowed probed
> instructions to fault, 
> do_page_fault()->kprobes_fault()->kprobe_fault_handler()
> - which uses the original probed address with search_exception_tables() to 
> find
> and run the fixup. I doubt this is needed in an initial version of kprobes,
> (maybe its later in this series - I haven't read all the way through it yet).

Hu..We do have fixup_exception() in arm64 kprobe_fault_handler(). So, it
should have worked, without this patch.

@David: This patch was added in v9 and fixup_exception() had been dropped in v9.
Since, dropping of fixup_exception() also caused to fail some systemtap test
cases, so it was added back in v10. I wonder if we really need this patch.
May be you can try to run related test case by dropping this patch. 

Thanks James for bringing this out.

~Pratyush

RE: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[平松雅巳 / HIRAMATU，MASAMI]

>From: "David A. Long" 
>
>Currrently taking exceptions when accessing user data from a kprobe'd
>instruction doesn't work. Avoid this situation by blacklisting the relevant
>functions.
>
>Signed-off-by: David A. Long 

Looks good to me.

Reviewed-by: Masami Hiramatsu 

Thanks,

>---
> arch/arm64/lib/copy_from_user.S | 1 +
> arch/arm64/lib/copy_to_user.S   | 1 +
> 2 files changed, 2 insertions(+)
>
>diff --git a/arch/arm64/lib/copy_from_user.S b/arch/arm64/lib/copy_from_user.S
>index 4699cd7..0ac2131 100644
>--- a/arch/arm64/lib/copy_from_user.S
>+++ b/arch/arm64/lib/copy_from_user.S
>@@ -66,6 +66,7 @@
>   .endm
>
> end   .reqx5
>+  .section .kprobes.text,"ax",%progbits
> ENTRY(__copy_from_user)
> ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \
>   CONFIG_ARM64_PAN)
>diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
>index 7512bbb..e4eb84c 100644
>--- a/arch/arm64/lib/copy_to_user.S
>+++ b/arch/arm64/lib/copy_to_user.S
>@@ -65,6 +65,7 @@
>   .endm
>
> end   .reqx5
>+  .section .kprobes.text,"ax",%progbits
> ENTRY(__copy_to_user)
> ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \
>   CONFIG_ARM64_PAN)
>--
>2.5.0
>
>
>___
>linux-arm-kernel mailing list
>linux-arm-ker...@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[平松雅巳 / HIRAMATU，MASAMI]

>From: "David A. Long" 
>
>Currrently taking exceptions when accessing user data from a kprobe'd
>instruction doesn't work. Avoid this situation by blacklisting the relevant
>functions.
>
>Signed-off-by: David A. Long 

Looks good to me.

Reviewed-by: Masami Hiramatsu 

Thanks,

>---
> arch/arm64/lib/copy_from_user.S | 1 +
> arch/arm64/lib/copy_to_user.S   | 1 +
> 2 files changed, 2 insertions(+)
>
>diff --git a/arch/arm64/lib/copy_from_user.S b/arch/arm64/lib/copy_from_user.S
>index 4699cd7..0ac2131 100644
>--- a/arch/arm64/lib/copy_from_user.S
>+++ b/arch/arm64/lib/copy_from_user.S
>@@ -66,6 +66,7 @@
>   .endm
>
> end   .reqx5
>+  .section .kprobes.text,"ax",%progbits
> ENTRY(__copy_from_user)
> ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \
>   CONFIG_ARM64_PAN)
>diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
>index 7512bbb..e4eb84c 100644
>--- a/arch/arm64/lib/copy_to_user.S
>+++ b/arch/arm64/lib/copy_to_user.S
>@@ -65,6 +65,7 @@
>   .endm
>
> end   .reqx5
>+  .section .kprobes.text,"ax",%progbits
> ENTRY(__copy_to_user)
> ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \
>   CONFIG_ARM64_PAN)
>--
>2.5.0
>
>
>___
>linux-arm-kernel mailing list
>linux-arm-ker...@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[Pratyush Anand]

Hi James,

On 18/03/2016:02:02:49 PM, James Morse wrote:
> Hi Pratyush,
> 
> On 18/03/16 13:29, Pratyush Anand wrote:
> > Probably, I can see why does not it work. So, when we are single stepping an
> > instruction and page fault occurs, we will come to el1_da in entry.S. Here, 
> > we
> > do enable_dbg. As soon as we will do this, we will start receiving single 
> > step
> > exception after each instruction (not sure, probably for each alternate
> > instruction). Since, there will not be any matching single step handler for
> > these instructions, so we will see warning "Unexpected kernel single-step
> > exception at EL1". 
> > 
> > So, I think, we should 
> > 
> > (1) may be do not enable debug for el1_da, or
> > (2) enable_dbg only when single stepping is not enabled, or
> > (3) or disable single stepping during el1_da execution.
> > 
> > (1) will solve the issue for sure, but not sure if it could be the best 
> > choice.
> 
> A variation on (3):
> 
> In kernel/entry.S when entered from EL0 we test for TIF_SINGLESTEP in the
> thread_info flags, and use disable_step_tsk/enable_step_tsk to save/restore 
> the
> single-step state.
> 
> Could we do this regardless of which EL we came from?

Thanks for another idea. I think, we can not do this as it is, because
TIF_SINGLESTEP will not be set for kprobe events. But, we can introduce a
variant disable_step_kernel and enable_step_kernel, which can be called in
el1_da. 

I will write a test case to reproduce the issue without this patch, and then
will do test with a patch based on something like above.

~Pratyush

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[Pratyush Anand]

Hi James,

On 18/03/2016:02:02:49 PM, James Morse wrote:
> Hi Pratyush,
> 
> On 18/03/16 13:29, Pratyush Anand wrote:
> > Probably, I can see why does not it work. So, when we are single stepping an
> > instruction and page fault occurs, we will come to el1_da in entry.S. Here, 
> > we
> > do enable_dbg. As soon as we will do this, we will start receiving single 
> > step
> > exception after each instruction (not sure, probably for each alternate
> > instruction). Since, there will not be any matching single step handler for
> > these instructions, so we will see warning "Unexpected kernel single-step
> > exception at EL1". 
> > 
> > So, I think, we should 
> > 
> > (1) may be do not enable debug for el1_da, or
> > (2) enable_dbg only when single stepping is not enabled, or
> > (3) or disable single stepping during el1_da execution.
> > 
> > (1) will solve the issue for sure, but not sure if it could be the best 
> > choice.
> 
> A variation on (3):
> 
> In kernel/entry.S when entered from EL0 we test for TIF_SINGLESTEP in the
> thread_info flags, and use disable_step_tsk/enable_step_tsk to save/restore 
> the
> single-step state.
> 
> Could we do this regardless of which EL we came from?

Thanks for another idea. I think, we can not do this as it is, because
TIF_SINGLESTEP will not be set for kprobe events. But, we can introduce a
variant disable_step_kernel and enable_step_kernel, which can be called in
el1_da. 

I will write a test case to reproduce the issue without this patch, and then
will do test with a patch based on something like above.

~Pratyush

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[James Morse]

Hi Pratyush,

On 18/03/16 14:43, Pratyush Anand wrote:
> On 18/03/2016:02:02:49 PM, James Morse wrote:
>> In kernel/entry.S when entered from EL0 we test for TIF_SINGLESTEP in the
>> thread_info flags, and use disable_step_tsk/enable_step_tsk to save/restore 
>> the
>> single-step state.
>>
>> Could we do this regardless of which EL we came from?
> 
> Thanks for another idea. I think, we can not do this as it is, because
> TIF_SINGLESTEP will not be set for kprobe events.

Hmmm, I see kernel_enable_single_step() doesn't set it, but setup_singlestep()
in patch 5 could...

There is probably a good reason its never set for a kernel thread, I will have a
look at where else it is used.


> But, we can introduce a
> variant disable_step_kernel and enable_step_kernel, which can be called in
> el1_da. 

What about sp/pc misalignment, or undefined instructions?
Or worse... an irq occurs during your el1_da call (el1_da may re-enable irqs).
el1_irq doesn't know you were careful not to unmask debug exceptions, it blindly
turns them back on.

The problem is the 'single step me' bit is still set, save/restoring it will
save us having to consider every interaction, (and then missing some!).

It would also mean you don't have to disable interrupts while single stepping in
patch 5 (comment above kprobes_save_local_irqflag()).


Thanks,

James

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[James Morse]

Hi Pratyush,

On 18/03/16 14:43, Pratyush Anand wrote:
> On 18/03/2016:02:02:49 PM, James Morse wrote:
>> In kernel/entry.S when entered from EL0 we test for TIF_SINGLESTEP in the
>> thread_info flags, and use disable_step_tsk/enable_step_tsk to save/restore 
>> the
>> single-step state.
>>
>> Could we do this regardless of which EL we came from?
> 
> Thanks for another idea. I think, we can not do this as it is, because
> TIF_SINGLESTEP will not be set for kprobe events.

Hmmm, I see kernel_enable_single_step() doesn't set it, but setup_singlestep()
in patch 5 could...

There is probably a good reason its never set for a kernel thread, I will have a
look at where else it is used.


> But, we can introduce a
> variant disable_step_kernel and enable_step_kernel, which can be called in
> el1_da. 

What about sp/pc misalignment, or undefined instructions?
Or worse... an irq occurs during your el1_da call (el1_da may re-enable irqs).
el1_irq doesn't know you were careful not to unmask debug exceptions, it blindly
turns them back on.

The problem is the 'single step me' bit is still set, save/restoring it will
save us having to consider every interaction, (and then missing some!).

It would also mean you don't have to disable interrupts while single stepping in
patch 5 (comment above kprobes_save_local_irqflag()).


Thanks,

James

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[Pratyush Anand]

On 17/03/2016:01:27:26 PM, Pratyush Anand wrote:
> @David: This patch was added in v9 and fixup_exception() had been dropped in 
> v9.
> Since, dropping of fixup_exception() also caused to fail some systemtap test
> cases, so it was added back in v10. I wonder if we really need this patch.
> May be you can try to run related test case by dropping this patch. 

Had a closer look to the code, and noticed that fixup_exception() does not have
any role in handling of page fault of copy_to_user(). Then, why do we have the
problem.
Probably, I can see why does not it work. So, when we are single stepping an
instruction and page fault occurs, we will come to el1_da in entry.S. Here, we
do enable_dbg. As soon as we will do this, we will start receiving single step
exception after each instruction (not sure, probably for each alternate
instruction). Since, there will not be any matching single step handler for
these instructions, so we will see warning "Unexpected kernel single-step
exception at EL1". 

So, I think, we should 

(1) may be do not enable debug for el1_da, or
(2) enable_dbg only when single stepping is not enabled, or
(3) or disable single stepping during el1_da execution.

(1) will solve the issue for sure, but not sure if it could be the best choice.

Will, what do you suggest?

~Pratyush

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[Pratyush Anand]

On 17/03/2016:01:27:26 PM, Pratyush Anand wrote:
> @David: This patch was added in v9 and fixup_exception() had been dropped in 
> v9.
> Since, dropping of fixup_exception() also caused to fail some systemtap test
> cases, so it was added back in v10. I wonder if we really need this patch.
> May be you can try to run related test case by dropping this patch. 

Had a closer look to the code, and noticed that fixup_exception() does not have
any role in handling of page fault of copy_to_user(). Then, why do we have the
problem.
Probably, I can see why does not it work. So, when we are single stepping an
instruction and page fault occurs, we will come to el1_da in entry.S. Here, we
do enable_dbg. As soon as we will do this, we will start receiving single step
exception after each instruction (not sure, probably for each alternate
instruction). Since, there will not be any matching single step handler for
these instructions, so we will see warning "Unexpected kernel single-step
exception at EL1". 

So, I think, we should 

(1) may be do not enable debug for el1_da, or
(2) enable_dbg only when single stepping is not enabled, or
(3) or disable single stepping during el1_da execution.

(1) will solve the issue for sure, but not sure if it could be the best choice.

Will, what do you suggest?

~Pratyush

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[James Morse]

Hi Pratyush,

On 16/03/16 05:43, Pratyush Anand wrote:
> On 15/03/2016:06:47:52 PM, James Morse wrote:
>> If I understand this correctly -  you can't kprobe these ldr/str instructions
>> as the fault handler wouldn't find kprobe's out-of line version of the
>> instruction in the exception table... but why only these two functions? (for
>> library functions, we also have clear_user() and copy_in_user()...)
>
> May be not clear_user() because those are inlined, but may be __clear_user().

You're right - the other library functions in that same directory is what I 
meant..

>> Is it feasible to search the exception table at runtime instead? If an
>> address-to-be-kprobed appears in the list, we know it could generate 
>> exceptions,
>> so we should report that we can't probe this address. That would catch all of
>> the library functions, all the places uaccess.h was inlined, and anything new
>> that gets invented in the future.
> 
> Sorry, probably I could not get it. How can an inlined addresses range be 
> placed
> in exception table or any other code area.

Ah, not a section or code area, sorry I wasn't clear:

When a fault happens in the kernel, the fault handler
(/arch/arm64/mm/fault.c:do_page_fault()) calls search_exception_tables(regs->pc)
to see if the faulting address has a 'fixup' registered. If it does, the fixup
causes -EFAULT to be returned, if not it ends up in die().

The horrible block of assembler in
arch/arm64/include/asm/uaccess.h:__get_user_asm() adds the address of the
instruction that is allowed to fault to the __ex_table section:
>   .section __ex_table,"a"
>   .align  3
>   .quad   1b, 3b
>   .previous

Here 1b is the address of the instruction that can fault, and 3b is the fixup
that moves -EFAULT into the return value.

This works for get_user() and friends which are inlined all over the kernel. It
even works for modules, as there is an exception table for each module which is
searched by kernel/module.c:search_module_extables().

This list of addresses that can fault already exists, there is even an API
function to check for a given address. Grabbing the nearest vmlinux, there are
~1300 entries in the __ex_table section, this patch blacklists two of them,
using search_exception_tables() obviously blacklists them all.


I've had a quick look at x86 and sparc, it looks like they allowed probed
instructions to fault, do_page_fault()->kprobes_fault()->kprobe_fault_handler()
- which uses the original probed address with search_exception_tables() to find
and run the fixup. I doubt this is needed in an initial version of kprobes,
(maybe its later in this series - I haven't read all the way through it yet).


Thanks,

James

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[James Morse]

Hi Pratyush,

On 16/03/16 05:43, Pratyush Anand wrote:
> On 15/03/2016:06:47:52 PM, James Morse wrote:
>> If I understand this correctly -  you can't kprobe these ldr/str instructions
>> as the fault handler wouldn't find kprobe's out-of line version of the
>> instruction in the exception table... but why only these two functions? (for
>> library functions, we also have clear_user() and copy_in_user()...)
>
> May be not clear_user() because those are inlined, but may be __clear_user().

You're right - the other library functions in that same directory is what I 
meant..

>> Is it feasible to search the exception table at runtime instead? If an
>> address-to-be-kprobed appears in the list, we know it could generate 
>> exceptions,
>> so we should report that we can't probe this address. That would catch all of
>> the library functions, all the places uaccess.h was inlined, and anything new
>> that gets invented in the future.
> 
> Sorry, probably I could not get it. How can an inlined addresses range be 
> placed
> in exception table or any other code area.

Ah, not a section or code area, sorry I wasn't clear:

When a fault happens in the kernel, the fault handler
(/arch/arm64/mm/fault.c:do_page_fault()) calls search_exception_tables(regs->pc)
to see if the faulting address has a 'fixup' registered. If it does, the fixup
causes -EFAULT to be returned, if not it ends up in die().

The horrible block of assembler in
arch/arm64/include/asm/uaccess.h:__get_user_asm() adds the address of the
instruction that is allowed to fault to the __ex_table section:
>   .section __ex_table,"a"
>   .align  3
>   .quad   1b, 3b
>   .previous

Here 1b is the address of the instruction that can fault, and 3b is the fixup
that moves -EFAULT into the return value.

This works for get_user() and friends which are inlined all over the kernel. It
even works for modules, as there is an exception table for each module which is
searched by kernel/module.c:search_module_extables().

This list of addresses that can fault already exists, there is even an API
function to check for a given address. Grabbing the nearest vmlinux, there are
~1300 entries in the __ex_table section, this patch blacklists two of them,
using search_exception_tables() obviously blacklists them all.


I've had a quick look at x86 and sparc, it looks like they allowed probed
instructions to fault, do_page_fault()->kprobes_fault()->kprobe_fault_handler()
- which uses the original probed address with search_exception_tables() to find
and run the fixup. I doubt this is needed in an initial version of kprobes,
(maybe its later in this series - I haven't read all the way through it yet).


Thanks,

James

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[Pratyush Anand]

On 15/03/2016:06:47:52 PM, James Morse wrote:
> Hi David,
> 
> On 09/03/16 05:32, David Long wrote:
> > From: "David A. Long" 
> > diff --git a/arch/arm64/lib/copy_from_user.S 
> > b/arch/arm64/lib/copy_from_user.S
> > index 4699cd7..0ac2131 100644
> > --- a/arch/arm64/lib/copy_from_user.S
> > +++ b/arch/arm64/lib/copy_from_user.S
> > @@ -66,6 +66,7 @@
> > .endm
> >  
> >  end.reqx5
> > +   .section .kprobes.text,"ax",%progbits
> >  ENTRY(__copy_from_user)
> >  ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \
> > CONFIG_ARM64_PAN)
> > diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
> > index 7512bbb..e4eb84c 100644
> > --- a/arch/arm64/lib/copy_to_user.S
> > +++ b/arch/arm64/lib/copy_to_user.S
> > @@ -65,6 +65,7 @@
> > .endm
> >  
> >  end.reqx5
> > +   .section .kprobes.text,"ax",%progbits
> >  ENTRY(__copy_to_user)
> >  ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \
> > CONFIG_ARM64_PAN)
> > 
> 
> If I understand this correctly -  you can't kprobe these ldr/str instructions 
> as
> the fault handler wouldn't find kprobe's out-of line version of the 
> instruction
> in the exception table... but why only these two functions? (for library
> functions, we also have clear_user() and copy_in_user()...)

May be not clear_user() because those are inlined, but may be __clear_user().

There can be many other functions (see [1], [2] and can be many more) which need
to be blacklisted, but I think they can always be added latter on, and atleast
this aspect should not hinder inclusion of these patches.

> 
> The get_user()/put_user() stuff in uaccess.h gets inlined all over the 
> kernel, I
> don't think its feasible to put all of these in a separate section.

Yes, It does not seem possible to blacklist  inlined functions. There can be
some other places like valid kprobable instructions in atomic context, .word
instruction having data as valid instruction, etc... So, probably its not
possible to make 100% safe, but yes wherever possible, we should take care.

Infact, other ARCHs are also not completely safe. One can try to instrument
kprobe on all the symbols in Kallsyms on an x86_64 machine and kernel crashes.

> 
> Is it feasible to search the exception table at runtime instead? If an
> address-to-be-kprobed appears in the list, we know it could generate 
> exceptions,
> so we should report that we can't probe this address. That would catch all of
> the library functions, all the places uaccess.h was inlined, and anything new
> that gets invented in the future.

Sorry, probably I could not get it. How can an inlined addresses range be placed
in exception table or any other code area.

~Pratyush

[1] 
https://github.com/pratyushanand/linux/commit/855bc4dbb98ceafac4c933e00d203b1cd7ee9ca4
[2] 
https://github.com/pratyushanand/linux/commit/8bc586d6f767240e9ffa582f45a9ad11de47ecfb

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[Pratyush Anand]

On 15/03/2016:06:47:52 PM, James Morse wrote:
> Hi David,
> 
> On 09/03/16 05:32, David Long wrote:
> > From: "David A. Long" 
> > diff --git a/arch/arm64/lib/copy_from_user.S 
> > b/arch/arm64/lib/copy_from_user.S
> > index 4699cd7..0ac2131 100644
> > --- a/arch/arm64/lib/copy_from_user.S
> > +++ b/arch/arm64/lib/copy_from_user.S
> > @@ -66,6 +66,7 @@
> > .endm
> >  
> >  end.reqx5
> > +   .section .kprobes.text,"ax",%progbits
> >  ENTRY(__copy_from_user)
> >  ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \
> > CONFIG_ARM64_PAN)
> > diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
> > index 7512bbb..e4eb84c 100644
> > --- a/arch/arm64/lib/copy_to_user.S
> > +++ b/arch/arm64/lib/copy_to_user.S
> > @@ -65,6 +65,7 @@
> > .endm
> >  
> >  end.reqx5
> > +   .section .kprobes.text,"ax",%progbits
> >  ENTRY(__copy_to_user)
> >  ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \
> > CONFIG_ARM64_PAN)
> > 
> 
> If I understand this correctly -  you can't kprobe these ldr/str instructions 
> as
> the fault handler wouldn't find kprobe's out-of line version of the 
> instruction
> in the exception table... but why only these two functions? (for library
> functions, we also have clear_user() and copy_in_user()...)

May be not clear_user() because those are inlined, but may be __clear_user().

There can be many other functions (see [1], [2] and can be many more) which need
to be blacklisted, but I think they can always be added latter on, and atleast
this aspect should not hinder inclusion of these patches.

> 
> The get_user()/put_user() stuff in uaccess.h gets inlined all over the 
> kernel, I
> don't think its feasible to put all of these in a separate section.

Yes, It does not seem possible to blacklist  inlined functions. There can be
some other places like valid kprobable instructions in atomic context, .word
instruction having data as valid instruction, etc... So, probably its not
possible to make 100% safe, but yes wherever possible, we should take care.

Infact, other ARCHs are also not completely safe. One can try to instrument
kprobe on all the symbols in Kallsyms on an x86_64 machine and kernel crashes.

> 
> Is it feasible to search the exception table at runtime instead? If an
> address-to-be-kprobed appears in the list, we know it could generate 
> exceptions,
> so we should report that we can't probe this address. That would catch all of
> the library functions, all the places uaccess.h was inlined, and anything new
> that gets invented in the future.

Sorry, probably I could not get it. How can an inlined addresses range be placed
in exception table or any other code area.

~Pratyush

[1] 
https://github.com/pratyushanand/linux/commit/855bc4dbb98ceafac4c933e00d203b1cd7ee9ca4
[2] 
https://github.com/pratyushanand/linux/commit/8bc586d6f767240e9ffa582f45a9ad11de47ecfb

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[James Morse]

Hi David,

On 09/03/16 05:32, David Long wrote:
> From: "David A. Long" 
> diff --git a/arch/arm64/lib/copy_from_user.S b/arch/arm64/lib/copy_from_user.S
> index 4699cd7..0ac2131 100644
> --- a/arch/arm64/lib/copy_from_user.S
> +++ b/arch/arm64/lib/copy_from_user.S
> @@ -66,6 +66,7 @@
>   .endm
>  
>  end  .reqx5
> + .section .kprobes.text,"ax",%progbits
>  ENTRY(__copy_from_user)
>  ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \
>   CONFIG_ARM64_PAN)
> diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
> index 7512bbb..e4eb84c 100644
> --- a/arch/arm64/lib/copy_to_user.S
> +++ b/arch/arm64/lib/copy_to_user.S
> @@ -65,6 +65,7 @@
>   .endm
>  
>  end  .reqx5
> + .section .kprobes.text,"ax",%progbits
>  ENTRY(__copy_to_user)
>  ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \
>   CONFIG_ARM64_PAN)
> 

If I understand this correctly -  you can't kprobe these ldr/str instructions as
the fault handler wouldn't find kprobe's out-of line version of the instruction
in the exception table... but why only these two functions? (for library
functions, we also have clear_user() and copy_in_user()...)

The get_user()/put_user() stuff in uaccess.h gets inlined all over the kernel, I
don't think its feasible to put all of these in a separate section.

Is it feasible to search the exception table at runtime instead? If an
address-to-be-kprobed appears in the list, we know it could generate exceptions,
so we should report that we can't probe this address. That would catch all of
the library functions, all the places uaccess.h was inlined, and anything new
that gets invented in the future.


> Currrently taking exceptions when accessing user data from a kprobe'd

(Nit: Currently)


Thanks,

James

Re: [PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[James Morse]

Hi David,

On 09/03/16 05:32, David Long wrote:
> From: "David A. Long" 
> diff --git a/arch/arm64/lib/copy_from_user.S b/arch/arm64/lib/copy_from_user.S
> index 4699cd7..0ac2131 100644
> --- a/arch/arm64/lib/copy_from_user.S
> +++ b/arch/arm64/lib/copy_from_user.S
> @@ -66,6 +66,7 @@
>   .endm
>  
>  end  .reqx5
> + .section .kprobes.text,"ax",%progbits
>  ENTRY(__copy_from_user)
>  ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \
>   CONFIG_ARM64_PAN)
> diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
> index 7512bbb..e4eb84c 100644
> --- a/arch/arm64/lib/copy_to_user.S
> +++ b/arch/arm64/lib/copy_to_user.S
> @@ -65,6 +65,7 @@
>   .endm
>  
>  end  .reqx5
> + .section .kprobes.text,"ax",%progbits
>  ENTRY(__copy_to_user)
>  ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \
>   CONFIG_ARM64_PAN)
> 

If I understand this correctly -  you can't kprobe these ldr/str instructions as
the fault handler wouldn't find kprobe's out-of line version of the instruction
in the exception table... but why only these two functions? (for library
functions, we also have clear_user() and copy_in_user()...)

The get_user()/put_user() stuff in uaccess.h gets inlined all over the kernel, I
don't think its feasible to put all of these in a separate section.

Is it feasible to search the exception table at runtime instead? If an
address-to-be-kprobed appears in the list, we know it could generate exceptions,
so we should report that we can't probe this address. That would catch all of
the library functions, all the places uaccess.h was inlined, and anything new
that gets invented in the future.


> Currrently taking exceptions when accessing user data from a kprobe'd

(Nit: Currently)


Thanks,

James

[PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[David Long]

From: "David A. Long" 

Currrently taking exceptions when accessing user data from a kprobe'd
instruction doesn't work. Avoid this situation by blacklisting the relevant
functions.

Signed-off-by: David A. Long 
---
 arch/arm64/lib/copy_from_user.S | 1 +
 arch/arm64/lib/copy_to_user.S   | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/arm64/lib/copy_from_user.S b/arch/arm64/lib/copy_from_user.S
index 4699cd7..0ac2131 100644
--- a/arch/arm64/lib/copy_from_user.S
+++ b/arch/arm64/lib/copy_from_user.S
@@ -66,6 +66,7 @@
.endm
 
 end.reqx5
+   .section .kprobes.text,"ax",%progbits
 ENTRY(__copy_from_user)
 ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \
CONFIG_ARM64_PAN)
diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
index 7512bbb..e4eb84c 100644
--- a/arch/arm64/lib/copy_to_user.S
+++ b/arch/arm64/lib/copy_to_user.S
@@ -65,6 +65,7 @@
.endm
 
 end.reqx5
+   .section .kprobes.text,"ax",%progbits
 ENTRY(__copy_to_user)
 ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \
CONFIG_ARM64_PAN)
-- 
2.5.0

[PATCH v11 3/9] arm64: add copy_to/from_user to kprobes blacklist

[David Long]

From: "David A. Long" 

Currrently taking exceptions when accessing user data from a kprobe'd
instruction doesn't work. Avoid this situation by blacklisting the relevant
functions.

Signed-off-by: David A. Long 
---
 arch/arm64/lib/copy_from_user.S | 1 +
 arch/arm64/lib/copy_to_user.S   | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/arm64/lib/copy_from_user.S b/arch/arm64/lib/copy_from_user.S
index 4699cd7..0ac2131 100644
--- a/arch/arm64/lib/copy_from_user.S
+++ b/arch/arm64/lib/copy_from_user.S
@@ -66,6 +66,7 @@
.endm
 
 end.reqx5
+   .section .kprobes.text,"ax",%progbits
 ENTRY(__copy_from_user)
 ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \
CONFIG_ARM64_PAN)
diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
index 7512bbb..e4eb84c 100644
--- a/arch/arm64/lib/copy_to_user.S
+++ b/arch/arm64/lib/copy_to_user.S
@@ -65,6 +65,7 @@
.endm
 
 end.reqx5
+   .section .kprobes.text,"ax",%progbits
 ENTRY(__copy_to_user)
 ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \
CONFIG_ARM64_PAN)
-- 
2.5.0