| Message ID | 701603cefa05559fec722e6cb809ae6afd0648e6.1498069502.git.naveen.n.rao@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, 22 Jun 2017 00:08:37 +0530 "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > Currently, we assume that the function pointer we receive in > ppc_function_entry() points to a function descriptor. However, this is > not always the case. In particular, assembly symbols without the right > annotation do not have an associated function descriptor. Some of these > symbols are added to the kprobe blacklist using _ASM_NOKPROBE_SYMBOL(). > When such addresses are subsequently processed through > arch_deref_entry_point() in populate_kprobe_blacklist(), we see the > below errors during bootup: > [ 0.663963] Failed to find blacklist at 7d9b02a648029b6c > [ 0.663970] Failed to find blacklist at a14d03d0394a0001 > [ 0.663972] Failed to find blacklist at 7d5302a6f94d0388 > [ 0.663973] Failed to find blacklist at 48027d11e8610178 > [ 0.663974] Failed to find blacklist at f8010070f8410080 > [ 0.663976] Failed to find blacklist at 386100704801f89d > [ 0.663977] Failed to find blacklist at 7d5302a6f94d00b0 > > Fix this by checking if the address in the function descriptor is > actually a valid kernel address. In the case of assembly symbols, this > will almost always fail as this ends up being powerpc instructions. In > that case, return pointer to the address we received, rather than the > dereferenced value. > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > --- > arch/powerpc/include/asm/code-patching.h | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h > index abef812de7f8..ec54050be585 100644 > --- a/arch/powerpc/include/asm/code-patching.h > +++ b/arch/powerpc/include/asm/code-patching.h > @@ -83,8 +83,16 @@ static inline unsigned long ppc_function_entry(void *func) > * On PPC64 ABIv1 the function pointer actually points to the > * function's descriptor. The first entry in the descriptor is the > * address of the function text. > + * > + * However, we may have received a pointer to an assembly symbol > + * that may not be a function descriptor. Validate that the entry > + * points to a valid kernel address and if not, return the pointer > + * we received as is. > */ > - return ((func_descr_t *)func)->entry; > + if (kernel_text_address(((func_descr_t *)func)->entry)) > + return ((func_descr_t *)func)->entry; > + else > + return (unsigned long)func; What if "func" is a text section label (bare asm function)? Won't func->entry load the random instruction located there and compare it with a kernel address? I don't know too much about the v1 ABI, but should we check for func belonging in the .opd section first and base the check on that? Alternatively I if "func" is in the kernel text address, we can recognize it's not in the .opd section... right? Thanks, Nick
Nicholas Piggin <npiggin@gmail.com> writes: > On Thu, 22 Jun 2017 00:08:37 +0530 > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > >> Currently, we assume that the function pointer we receive in >> ppc_function_entry() points to a function descriptor. However, this is >> not always the case. In particular, assembly symbols without the right >> annotation do not have an associated function descriptor. Some of these >> symbols are added to the kprobe blacklist using _ASM_NOKPROBE_SYMBOL(). >> When such addresses are subsequently processed through >> arch_deref_entry_point() in populate_kprobe_blacklist(), we see the >> below errors during bootup: >> [ 0.663963] Failed to find blacklist at 7d9b02a648029b6c >> [ 0.663970] Failed to find blacklist at a14d03d0394a0001 >> [ 0.663972] Failed to find blacklist at 7d5302a6f94d0388 >> [ 0.663973] Failed to find blacklist at 48027d11e8610178 >> [ 0.663974] Failed to find blacklist at f8010070f8410080 >> [ 0.663976] Failed to find blacklist at 386100704801f89d >> [ 0.663977] Failed to find blacklist at 7d5302a6f94d00b0 >> >> Fix this by checking if the address in the function descriptor is >> actually a valid kernel address. In the case of assembly symbols, this >> will almost always fail as this ends up being powerpc instructions. In >> that case, return pointer to the address we received, rather than the >> dereferenced value. >> >> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> >> --- >> arch/powerpc/include/asm/code-patching.h | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h >> index abef812de7f8..ec54050be585 100644 >> --- a/arch/powerpc/include/asm/code-patching.h >> +++ b/arch/powerpc/include/asm/code-patching.h >> @@ -83,8 +83,16 @@ static inline unsigned long ppc_function_entry(void *func) >> * On PPC64 ABIv1 the function pointer actually points to the >> * function's descriptor. The first entry in the descriptor is the >> * address of the function text. >> + * >> + * However, we may have received a pointer to an assembly symbol >> + * that may not be a function descriptor. Validate that the entry >> + * points to a valid kernel address and if not, return the pointer >> + * we received as is. >> */ >> - return ((func_descr_t *)func)->entry; >> + if (kernel_text_address(((func_descr_t *)func)->entry)) >> + return ((func_descr_t *)func)->entry; >> + else >> + return (unsigned long)func; > > What if "func" is a text section label (bare asm function)? > Won't func->entry load the random instruction located there > and compare it with a kernel address? Yes, that's the problem. > I don't know too much about the v1 ABI, but should we check for > func belonging in the .opd section first and base the check on > that? Alternatively I if "func" is in the kernel text address, > we can recognize it's not in the .opd section... right? That sounds like a more robust solution. But I suspect it won't work for modules. Another option might be to canonicalise the blacklist so that it always points to the text address, not sure how easy that would be. cheers
On Thu, 22 Jun 2017 20:59:49 +1000 Michael Ellerman <mpe@ellerman.id.au> wrote: > Nicholas Piggin <npiggin@gmail.com> writes: > > > On Thu, 22 Jun 2017 00:08:37 +0530 > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > > > >> Currently, we assume that the function pointer we receive in > >> ppc_function_entry() points to a function descriptor. However, this is > >> not always the case. In particular, assembly symbols without the right > >> annotation do not have an associated function descriptor. Some of these > >> symbols are added to the kprobe blacklist using _ASM_NOKPROBE_SYMBOL(). > >> When such addresses are subsequently processed through > >> arch_deref_entry_point() in populate_kprobe_blacklist(), we see the > >> below errors during bootup: > >> [ 0.663963] Failed to find blacklist at 7d9b02a648029b6c > >> [ 0.663970] Failed to find blacklist at a14d03d0394a0001 > >> [ 0.663972] Failed to find blacklist at 7d5302a6f94d0388 > >> [ 0.663973] Failed to find blacklist at 48027d11e8610178 > >> [ 0.663974] Failed to find blacklist at f8010070f8410080 > >> [ 0.663976] Failed to find blacklist at 386100704801f89d > >> [ 0.663977] Failed to find blacklist at 7d5302a6f94d00b0 > >> > >> Fix this by checking if the address in the function descriptor is > >> actually a valid kernel address. In the case of assembly symbols, this > >> will almost always fail as this ends up being powerpc instructions. In > >> that case, return pointer to the address we received, rather than the > >> dereferenced value. > >> > >> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > >> --- > >> arch/powerpc/include/asm/code-patching.h | 10 +++++++++- > >> 1 file changed, 9 insertions(+), 1 deletion(-) > >> > >> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h > >> index abef812de7f8..ec54050be585 100644 > >> --- a/arch/powerpc/include/asm/code-patching.h > >> +++ b/arch/powerpc/include/asm/code-patching.h > >> @@ -83,8 +83,16 @@ static inline unsigned long ppc_function_entry(void *func) > >> * On PPC64 ABIv1 the function pointer actually points to the > >> * function's descriptor. The first entry in the descriptor is the > >> * address of the function text. > >> + * > >> + * However, we may have received a pointer to an assembly symbol > >> + * that may not be a function descriptor. Validate that the entry > >> + * points to a valid kernel address and if not, return the pointer > >> + * we received as is. > >> */ > >> - return ((func_descr_t *)func)->entry; > >> + if (kernel_text_address(((func_descr_t *)func)->entry)) > >> + return ((func_descr_t *)func)->entry; > >> + else > >> + return (unsigned long)func; > > > > What if "func" is a text section label (bare asm function)? > > Won't func->entry load the random instruction located there > > and compare it with a kernel address? > > Yes, that's the problem. > > > I don't know too much about the v1 ABI, but should we check for > > func belonging in the .opd section first and base the check on > > that? Alternatively I if "func" is in the kernel text address, > > we can recognize it's not in the .opd section... right? > > That sounds like a more robust solution. But I suspect it won't work for > modules. kernel_text_address() seems to check for module text as well, so it might work I think?
On 2017/06/22 11:06PM, Nicholas Piggin wrote: > On Thu, 22 Jun 2017 20:59:49 +1000 > Michael Ellerman <mpe@ellerman.id.au> wrote: > > > Nicholas Piggin <npiggin@gmail.com> writes: > > > > > On Thu, 22 Jun 2017 00:08:37 +0530 > > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > > > > > >> Currently, we assume that the function pointer we receive in > > >> ppc_function_entry() points to a function descriptor. However, this is > > >> not always the case. In particular, assembly symbols without the right > > >> annotation do not have an associated function descriptor. Some of these > > >> symbols are added to the kprobe blacklist using _ASM_NOKPROBE_SYMBOL(). > > >> When such addresses are subsequently processed through > > >> arch_deref_entry_point() in populate_kprobe_blacklist(), we see the > > >> below errors during bootup: > > >> [ 0.663963] Failed to find blacklist at 7d9b02a648029b6c > > >> [ 0.663970] Failed to find blacklist at a14d03d0394a0001 > > >> [ 0.663972] Failed to find blacklist at 7d5302a6f94d0388 > > >> [ 0.663973] Failed to find blacklist at 48027d11e8610178 > > >> [ 0.663974] Failed to find blacklist at f8010070f8410080 > > >> [ 0.663976] Failed to find blacklist at 386100704801f89d > > >> [ 0.663977] Failed to find blacklist at 7d5302a6f94d00b0 > > >> > > >> Fix this by checking if the address in the function descriptor is > > >> actually a valid kernel address. In the case of assembly symbols, this > > >> will almost always fail as this ends up being powerpc instructions. In > > >> that case, return pointer to the address we received, rather than the > > >> dereferenced value. > > >> > > >> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > > >> --- > > >> arch/powerpc/include/asm/code-patching.h | 10 +++++++++- > > >> 1 file changed, 9 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h > > >> index abef812de7f8..ec54050be585 100644 > > >> --- a/arch/powerpc/include/asm/code-patching.h > > >> +++ b/arch/powerpc/include/asm/code-patching.h > > >> @@ -83,8 +83,16 @@ static inline unsigned long ppc_function_entry(void *func) > > >> * On PPC64 ABIv1 the function pointer actually points to the > > >> * function's descriptor. The first entry in the descriptor is the > > >> * address of the function text. > > >> + * > > >> + * However, we may have received a pointer to an assembly symbol > > >> + * that may not be a function descriptor. Validate that the entry > > >> + * points to a valid kernel address and if not, return the pointer > > >> + * we received as is. > > >> */ > > >> - return ((func_descr_t *)func)->entry; > > >> + if (kernel_text_address(((func_descr_t *)func)->entry)) > > >> + return ((func_descr_t *)func)->entry; > > >> + else > > >> + return (unsigned long)func; > > > > > > What if "func" is a text section label (bare asm function)? > > > Won't func->entry load the random instruction located there > > > and compare it with a kernel address? > > > > Yes, that's the problem. Yes, we were currently returning those instructions as the function entry address. > > > > > I don't know too much about the v1 ABI, but should we check for > > > func belonging in the .opd section first and base the check on > > > that? Alternatively I if "func" is in the kernel text address, > > > we can recognize it's not in the .opd section... right? > > > > That sounds like a more robust solution. But I suspect it won't work for > > modules. > > kernel_text_address() seems to check for module text as well, so it > might work I think? Yes, I think that's a very nice idea! I'll check and confirm that it does what it's supposed to. Thanks for the review, - Naveen
diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h index abef812de7f8..ec54050be585 100644 --- a/arch/powerpc/include/asm/code-patching.h +++ b/arch/powerpc/include/asm/code-patching.h @@ -83,8 +83,16 @@ static inline unsigned long ppc_function_entry(void *func) * On PPC64 ABIv1 the function pointer actually points to the * function's descriptor. The first entry in the descriptor is the * address of the function text. + * + * However, we may have received a pointer to an assembly symbol + * that may not be a function descriptor. Validate that the entry + * points to a valid kernel address and if not, return the pointer + * we received as is. */ - return ((func_descr_t *)func)->entry; + if (kernel_text_address(((func_descr_t *)func)->entry)) + return ((func_descr_t *)func)->entry; + else + return (unsigned long)func; #else return (unsigned long)func; #endif
Currently, we assume that the function pointer we receive in ppc_function_entry() points to a function descriptor. However, this is not always the case. In particular, assembly symbols without the right annotation do not have an associated function descriptor. Some of these symbols are added to the kprobe blacklist using _ASM_NOKPROBE_SYMBOL(). When such addresses are subsequently processed through arch_deref_entry_point() in populate_kprobe_blacklist(), we see the below errors during bootup: [ 0.663963] Failed to find blacklist at 7d9b02a648029b6c [ 0.663970] Failed to find blacklist at a14d03d0394a0001 [ 0.663972] Failed to find blacklist at 7d5302a6f94d0388 [ 0.663973] Failed to find blacklist at 48027d11e8610178 [ 0.663974] Failed to find blacklist at f8010070f8410080 [ 0.663976] Failed to find blacklist at 386100704801f89d [ 0.663977] Failed to find blacklist at 7d5302a6f94d00b0 Fix this by checking if the address in the function descriptor is actually a valid kernel address. In the case of assembly symbols, this will almost always fail as this ends up being powerpc instructions. In that case, return pointer to the address we received, rather than the dereferenced value. Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> --- arch/powerpc/include/asm/code-patching.h | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)