* [PATCH] powerpc/kprobes: refactor kprobe_lookup_name for safer string operations
@ 2017-04-25 16:07 Naveen N. Rao
2017-04-26 4:41 ` Masami Hiramatsu
0 siblings, 1 reply; 4+ messages in thread
From: Naveen N. Rao @ 2017-04-25 16:07 UTC (permalink / raw)
To: Michael Ellerman
Cc: Masami Hiramatsu, David Laight, Paul Clarke,
Ananth N Mavinakayanahalli, linuxppc-dev
Use safer string manipulation functions when dealing with a
user-provided string in kprobe_lookup_name().
Reported-by: David Laight <David.Laight@ACULAB.COM>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/kernel/kprobes.c | 47 ++++++++++++++++++-------------------------
1 file changed, 20 insertions(+), 27 deletions(-)
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 160ae0fa7d0d..5a17e6467be9 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -53,7 +53,7 @@ bool arch_within_kprobe_blacklist(unsigned long addr)
kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
{
- kprobe_opcode_t *addr;
+ kprobe_opcode_t *addr = NULL;
#ifdef PPC64_ELF_ABI_v2
/* PPC64 ABIv2 needs local entry point */
@@ -85,36 +85,29 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
* Also handle <module:symbol> format.
*/
char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];
- const char *modsym;
bool dot_appended = false;
- if ((modsym = strchr(name, ':')) != NULL) {
- modsym++;
- if (*modsym != '\0' && *modsym != '.') {
- /* Convert to <module:.symbol> */
- strncpy(dot_name, name, modsym - name);
- dot_name[modsym - name] = '.';
- dot_name[modsym - name + 1] = '\0';
- strncat(dot_name, modsym,
- sizeof(dot_name) - (modsym - name) - 2);
- dot_appended = true;
- } else {
- dot_name[0] = '\0';
- strncat(dot_name, name, sizeof(dot_name) - 1);
- }
- } else if (name[0] != '.') {
- dot_name[0] = '.';
- dot_name[1] = '\0';
- strncat(dot_name, name, KSYM_NAME_LEN - 2);
+ const char *c;
+ ssize_t ret = 0;
+ int len = 0;
+
+ if ((c = strnchr(name, MODULE_NAME_LEN, ':')) != NULL) {
+ c++;
+ len = c - name;
+ memcpy(dot_name, name, len);
+ } else
+ c = name;
+
+ if (*c != '\0' && *c != '.') {
+ dot_name[len++] = '.';
dot_appended = true;
- } else {
- dot_name[0] = '\0';
- strncat(dot_name, name, KSYM_NAME_LEN - 1);
}
- addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
- if (!addr && dot_appended) {
- /* Let's try the original non-dot symbol lookup */
+ ret = strscpy(dot_name + len, c, KSYM_NAME_LEN);
+ if (ret >= 0)
+ addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
+
+ /* Fallback to the original non-dot symbol lookup */
+ if (!addr && dot_appended)
addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);
- }
#else
addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);
#endif
--
2.12.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] powerpc/kprobes: refactor kprobe_lookup_name for safer string operations
2017-04-25 16:07 [PATCH] powerpc/kprobes: refactor kprobe_lookup_name for safer string operations Naveen N. Rao
@ 2017-04-26 4:41 ` Masami Hiramatsu
2017-04-26 18:42 ` Naveen N. Rao
0 siblings, 1 reply; 4+ messages in thread
From: Masami Hiramatsu @ 2017-04-26 4:41 UTC (permalink / raw)
To: Naveen N. Rao
Cc: Michael Ellerman, Masami Hiramatsu, David Laight, Paul Clarke,
Ananth N Mavinakayanahalli, linuxppc-dev
On Tue, 25 Apr 2017 21:37:11 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> Use safer string manipulation functions when dealing with a
> user-provided string in kprobe_lookup_name().
>
> Reported-by: David Laight <David.Laight@ACULAB.COM>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> arch/powerpc/kernel/kprobes.c | 47 ++++++++++++++++++-------------------------
> 1 file changed, 20 insertions(+), 27 deletions(-)
>
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 160ae0fa7d0d..5a17e6467be9 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -53,7 +53,7 @@ bool arch_within_kprobe_blacklist(unsigned long addr)
>
> kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
> {
> - kprobe_opcode_t *addr;
> + kprobe_opcode_t *addr = NULL;
>
> #ifdef PPC64_ELF_ABI_v2
> /* PPC64 ABIv2 needs local entry point */
> @@ -85,36 +85,29 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
> * Also handle <module:symbol> format.
> */
> char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];
> - const char *modsym;
> bool dot_appended = false;
> - if ((modsym = strchr(name, ':')) != NULL) {
> - modsym++;
> - if (*modsym != '\0' && *modsym != '.') {
> - /* Convert to <module:.symbol> */
> - strncpy(dot_name, name, modsym - name);
> - dot_name[modsym - name] = '.';
> - dot_name[modsym - name + 1] = '\0';
> - strncat(dot_name, modsym,
> - sizeof(dot_name) - (modsym - name) - 2);
> - dot_appended = true;
> - } else {
> - dot_name[0] = '\0';
> - strncat(dot_name, name, sizeof(dot_name) - 1);
> - }
> - } else if (name[0] != '.') {
> - dot_name[0] = '.';
> - dot_name[1] = '\0';
> - strncat(dot_name, name, KSYM_NAME_LEN - 2);
> + const char *c;
> + ssize_t ret = 0;
> + int len = 0;
> +
> + if ((c = strnchr(name, MODULE_NAME_LEN, ':')) != NULL) {
> + c++;
> + len = c - name;
> + memcpy(dot_name, name, len);
> + } else
> + c = name;
> +
> + if (*c != '\0' && *c != '.') {
> + dot_name[len++] = '.';
> dot_appended = true;
> - } else {
> - dot_name[0] = '\0';
> - strncat(dot_name, name, KSYM_NAME_LEN - 1);
> }
> - addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
> - if (!addr && dot_appended) {
> - /* Let's try the original non-dot symbol lookup */
> + ret = strscpy(dot_name + len, c, KSYM_NAME_LEN);
> + if (ret >= 0)
Here, maybe you can skip the case of ret == 0. (Or, would we have
a symbol which only has "."?)
Others look good to me.
Thank you,
> + addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
> +
> + /* Fallback to the original non-dot symbol lookup */
> + if (!addr && dot_appended)
> addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);
> - }
> #else
> addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);
> #endif
> --
> 2.12.1
>
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] powerpc/kprobes: refactor kprobe_lookup_name for safer string operations
2017-04-26 4:41 ` Masami Hiramatsu
@ 2017-04-26 18:42 ` Naveen N. Rao
2017-04-27 11:31 ` Michael Ellerman
0 siblings, 1 reply; 4+ messages in thread
From: Naveen N. Rao @ 2017-04-26 18:42 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Ananth N Mavinakayanahalli, David Laight, linuxppc-dev,
Michael Ellerman, Paul Clarke
Excerpts from Masami Hiramatsu's message of April 26, 2017 10:11:
> On Tue, 25 Apr 2017 21:37:11 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
>=20
>> Use safer string manipulation functions when dealing with a
>> user-provided string in kprobe_lookup_name().
>>=20
>> Reported-by: David Laight <David.Laight@ACULAB.COM>
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/kernel/kprobes.c | 47 ++++++++++++++++++------------------=
-------
>> 1 file changed, 20 insertions(+), 27 deletions(-)
>>=20
>> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes=
.c
>> index 160ae0fa7d0d..5a17e6467be9 100644
>> --- a/arch/powerpc/kernel/kprobes.c
>> +++ b/arch/powerpc/kernel/kprobes.c
>> @@ -53,7 +53,7 @@ bool arch_within_kprobe_blacklist(unsigned long addr)
>> =20
>> kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offs=
et)
>> {
>> - kprobe_opcode_t *addr;
>> + kprobe_opcode_t *addr =3D NULL;
>> =20
>> #ifdef PPC64_ELF_ABI_v2
>> /* PPC64 ABIv2 needs local entry point */
>> @@ -85,36 +85,29 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name=
, unsigned int offset)
>> * Also handle <module:symbol> format.
>> */
>> char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];
>> - const char *modsym;
>> bool dot_appended =3D false;
>> - if ((modsym =3D strchr(name, ':')) !=3D NULL) {
>> - modsym++;
>> - if (*modsym !=3D '\0' && *modsym !=3D '.') {
>> - /* Convert to <module:.symbol> */
>> - strncpy(dot_name, name, modsym - name);
>> - dot_name[modsym - name] =3D '.';
>> - dot_name[modsym - name + 1] =3D '\0';
>> - strncat(dot_name, modsym,
>> - sizeof(dot_name) - (modsym - name) - 2);
>> - dot_appended =3D true;
>> - } else {
>> - dot_name[0] =3D '\0';
>> - strncat(dot_name, name, sizeof(dot_name) - 1);
>> - }
>> - } else if (name[0] !=3D '.') {
>> - dot_name[0] =3D '.';
>> - dot_name[1] =3D '\0';
>> - strncat(dot_name, name, KSYM_NAME_LEN - 2);
>> + const char *c;
>> + ssize_t ret =3D 0;
>> + int len =3D 0;
>> +
>> + if ((c =3D strnchr(name, MODULE_NAME_LEN, ':')) !=3D NULL) {
>> + c++;
>> + len =3D c - name;
>> + memcpy(dot_name, name, len);
>> + } else
>> + c =3D name;
>> +
>> + if (*c !=3D '\0' && *c !=3D '.') {
>> + dot_name[len++] =3D '.';
>> dot_appended =3D true;
>> - } else {
>> - dot_name[0] =3D '\0';
>> - strncat(dot_name, name, KSYM_NAME_LEN - 1);
>> }
>> - addr =3D (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
>> - if (!addr && dot_appended) {
>> - /* Let's try the original non-dot symbol lookup */
>> + ret =3D strscpy(dot_name + len, c, KSYM_NAME_LEN);
>> + if (ret >=3D 0)
>=20
> Here, maybe you can skip the case of ret =3D=3D 0. (Or, would we have
> a symbol which only has "."?)
Ah, indeed. Good point. We just need the test to be (ret > 0).
Michael,
If the rest of the patch is fine by you, would you be ok to make the=20
small change above? If not, please let me know and I'll re-spin. Thanks.
>=20
> Others look good to me.
Thanks for the review,
- Naveen
=
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] powerpc/kprobes: refactor kprobe_lookup_name for safer string operations
2017-04-26 18:42 ` Naveen N. Rao
@ 2017-04-27 11:31 ` Michael Ellerman
0 siblings, 0 replies; 4+ messages in thread
From: Michael Ellerman @ 2017-04-27 11:31 UTC (permalink / raw)
To: Naveen N. Rao, Masami Hiramatsu
Cc: Ananth N Mavinakayanahalli, David Laight, linuxppc-dev, Paul Clarke
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> Excerpts from Masami Hiramatsu's message of April 26, 2017 10:11:
>> On Tue, 25 Apr 2017 21:37:11 +0530
>> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
>>> - addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
>>> - if (!addr && dot_appended) {
>>> - /* Let's try the original non-dot symbol lookup */
>>> + ret = strscpy(dot_name + len, c, KSYM_NAME_LEN);
>>> + if (ret >= 0)
>>
>> Here, maybe you can skip the case of ret == 0. (Or, would we have
>> a symbol which only has "."?)
>
> Ah, indeed. Good point. We just need the test to be (ret > 0).
>
> Michael,
> If the rest of the patch is fine by you, would you be ok to make the
> small change above? If not, please let me know and I'll re-spin. Thanks.
I'd rather you change it, test and then resend.
cheers
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-04-27 11:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-25 16:07 [PATCH] powerpc/kprobes: refactor kprobe_lookup_name for safer string operations Naveen N. Rao
2017-04-26 4:41 ` Masami Hiramatsu
2017-04-26 18:42 ` Naveen N. Rao
2017-04-27 11:31 ` Michael Ellerman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.