* Re: [PATCH] kprobes: fix out-of-bounds in register_kretprobe
2021-12-01 5:48 [PATCH] kprobes: fix out-of-bounds in register_kretprobe zhangyue
@ 2021-12-01 13:00 ` Masami Hiramatsu
2021-12-01 13:06 ` Masami Hiramatsu
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Masami Hiramatsu @ 2021-12-01 13:00 UTC (permalink / raw)
To: zhangyue; +Cc: naveen.n.rao, anil.s.keshavamurthy, davem, linux-kernel
On Wed, 1 Dec 2021 13:48:55 +0800
zhangyue <zhangyue1@kylinos.cn> wrote:
> When the data 'rp->data_size' is negative, the code
> 'sizeof(struct kretprobe_instance)+rp->data_size'
> is less than 'sizeof(struct kretprobe_instance)'
>
> At this time, the pointer 'inst' may be out of
> bound when it is in use.
Good catch! but in that case register_kretprobe() should return -EINVAL
since there is no reason to allow minus data_size. (Thus, it must be
unsigned int, and limit with some maximum size.)
Let me fix that.
Thank you,
>
> Signed-off-by: zhangyue <zhangyue1@kylinos.cn>
> ---
> kernel/kprobes.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 1cf8bca1ea86..71cf6bde299f 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1983,7 +1983,7 @@ int kprobe_on_func_entry(kprobe_opcode_t *addr, const char *sym, unsigned long o
> int register_kretprobe(struct kretprobe *rp)
> {
> int ret;
> - struct kretprobe_instance *inst;
> + struct kretprobe_instance *inst = NULL;
> int i;
> void *addr;
>
> @@ -2024,7 +2024,8 @@ int register_kretprobe(struct kretprobe *rp)
>
> rp->rph->rp = rp;
> for (i = 0; i < rp->maxactive; i++) {
> - inst = kzalloc(sizeof(struct kretprobe_instance) +
> + if (rp->data_size >= 0)
> + inst = kzalloc(sizeof(struct kretprobe_instance) +
> rp->data_size, GFP_KERNEL);
> if (inst == NULL) {
> refcount_set(&rp->rph->ref, i);
> --
> 2.30.0
>
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] kprobes: fix out-of-bounds in register_kretprobe
2021-12-01 5:48 [PATCH] kprobes: fix out-of-bounds in register_kretprobe zhangyue
2021-12-01 13:00 ` Masami Hiramatsu
@ 2021-12-01 13:06 ` Masami Hiramatsu
2021-12-02 4:29 ` kernel test robot
2021-12-05 4:26 ` kernel test robot
3 siblings, 0 replies; 6+ messages in thread
From: Masami Hiramatsu @ 2021-12-01 13:06 UTC (permalink / raw)
To: zhangyue; +Cc: naveen.n.rao, anil.s.keshavamurthy, davem, linux-kernel
On Wed, 1 Dec 2021 13:48:55 +0800
zhangyue <zhangyue1@kylinos.cn> wrote:
> When the data 'rp->data_size' is negative, the code
> 'sizeof(struct kretprobe_instance)+rp->data_size'
> is less than 'sizeof(struct kretprobe_instance)'
Hmm, rp->data_size is size_t, which is unsigned value.
Of course we still need some kind of maximum limitation
because if we pass enough bigger size, the
sizeof(struct kretprobe_instance) + rp->data_size
can be negative or smaller than sizeof(struct kretprobe_instance).
Thank you,
>
> At this time, the pointer 'inst' may be out of
> bound when it is in use.
>
> Signed-off-by: zhangyue <zhangyue1@kylinos.cn>
> ---
> kernel/kprobes.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 1cf8bca1ea86..71cf6bde299f 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1983,7 +1983,7 @@ int kprobe_on_func_entry(kprobe_opcode_t *addr, const char *sym, unsigned long o
> int register_kretprobe(struct kretprobe *rp)
> {
> int ret;
> - struct kretprobe_instance *inst;
> + struct kretprobe_instance *inst = NULL;
> int i;
> void *addr;
>
> @@ -2024,7 +2024,8 @@ int register_kretprobe(struct kretprobe *rp)
>
> rp->rph->rp = rp;
> for (i = 0; i < rp->maxactive; i++) {
> - inst = kzalloc(sizeof(struct kretprobe_instance) +
> + if (rp->data_size >= 0)
> + inst = kzalloc(sizeof(struct kretprobe_instance) +
> rp->data_size, GFP_KERNEL);
> if (inst == NULL) {
> refcount_set(&rp->rph->ref, i);
> --
> 2.30.0
>
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] kprobes: fix out-of-bounds in register_kretprobe
2021-12-01 5:48 [PATCH] kprobes: fix out-of-bounds in register_kretprobe zhangyue
2021-12-01 13:00 ` Masami Hiramatsu
2021-12-01 13:06 ` Masami Hiramatsu
@ 2021-12-02 4:29 ` kernel test robot
2021-12-05 4:26 ` kernel test robot
3 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2021-12-02 4:29 UTC (permalink / raw)
To: zhangyue, naveen.n.rao, anil.s.keshavamurthy, davem, mhiramat
Cc: kbuild-all, linux-kernel
Hi zhangyue,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on rostedt-trace/for-next]
[also build test WARNING on v5.16-rc3 next-20211201]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/zhangyue/kprobes-fix-out-of-bounds-in-register_kretprobe/20211201-135046
base: https://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git for-next
config: powerpc64-randconfig-m031-20211129 (https://download.01.org/0day-ci/archive/20211202/202112021254.cDIRw2r6-lkp@intel.com/config)
compiler: powerpc64-linux-gcc (GCC) 11.2.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
smatch warnings:
kernel/kprobes.c:2107 register_kretprobe() warn: always true condition '(rp->data_size >= 0) => (0-u64max >= 0)'
vim +2107 kernel/kprobes.c
2062
2063 int register_kretprobe(struct kretprobe *rp)
2064 {
2065 int ret;
2066 struct kretprobe_instance *inst = NULL;
2067 int i;
2068 void *addr;
2069
2070 ret = kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset);
2071 if (ret)
2072 return ret;
2073
2074 /* If only 'rp->kp.addr' is specified, check reregistering kprobes */
2075 if (rp->kp.addr && warn_kprobe_rereg(&rp->kp))
2076 return -EINVAL;
2077
2078 if (kretprobe_blacklist_size) {
2079 addr = kprobe_addr(&rp->kp);
2080 if (IS_ERR(addr))
2081 return PTR_ERR(addr);
2082
2083 for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
2084 if (kretprobe_blacklist[i].addr == addr)
2085 return -EINVAL;
2086 }
2087 }
2088
2089 rp->kp.pre_handler = pre_handler_kretprobe;
2090 rp->kp.post_handler = NULL;
2091
2092 /* Pre-allocate memory for max kretprobe instances */
2093 if (rp->maxactive <= 0) {
2094 #ifdef CONFIG_PREEMPTION
2095 rp->maxactive = max_t(unsigned int, 10, 2*num_possible_cpus());
2096 #else
2097 rp->maxactive = num_possible_cpus();
2098 #endif
2099 }
2100 rp->freelist.head = NULL;
2101 rp->rph = kzalloc(sizeof(struct kretprobe_holder), GFP_KERNEL);
2102 if (!rp->rph)
2103 return -ENOMEM;
2104
2105 rp->rph->rp = rp;
2106 for (i = 0; i < rp->maxactive; i++) {
> 2107 if (rp->data_size >= 0)
2108 inst = kzalloc(sizeof(struct kretprobe_instance) +
2109 rp->data_size, GFP_KERNEL);
2110 if (inst == NULL) {
2111 refcount_set(&rp->rph->ref, i);
2112 free_rp_inst(rp);
2113 return -ENOMEM;
2114 }
2115 inst->rph = rp->rph;
2116 freelist_add(&inst->freelist, &rp->freelist);
2117 }
2118 refcount_set(&rp->rph->ref, i);
2119
2120 rp->nmissed = 0;
2121 /* Establish function entry probe point */
2122 ret = register_kprobe(&rp->kp);
2123 if (ret != 0)
2124 free_rp_inst(rp);
2125 return ret;
2126 }
2127 EXPORT_SYMBOL_GPL(register_kretprobe);
2128
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] kprobes: fix out-of-bounds in register_kretprobe
2021-12-01 5:48 [PATCH] kprobes: fix out-of-bounds in register_kretprobe zhangyue
` (2 preceding siblings ...)
2021-12-02 4:29 ` kernel test robot
@ 2021-12-05 4:26 ` kernel test robot
2021-12-06 0:22 ` Masami Hiramatsu
3 siblings, 1 reply; 6+ messages in thread
From: kernel test robot @ 2021-12-05 4:26 UTC (permalink / raw)
To: zhangyue, naveen.n.rao, anil.s.keshavamurthy, davem, mhiramat
Cc: kbuild-all, linux-kernel
Hi zhangyue,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on rostedt-trace/for-next]
[also build test WARNING on v5.16-rc3 next-20211203]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/zhangyue/kprobes-fix-out-of-bounds-in-register_kretprobe/20211201-135046
base: https://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git for-next
config: i386-randconfig-m021-20211203 (https://download.01.org/0day-ci/archive/20211205/202112051255.NQeIOpp8-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
smatch warnings:
kernel/kprobes.c:2107 register_kretprobe() warn: always true condition '(rp->data_size >= 0) => (0-u32max >= 0)'
vim +2107 kernel/kprobes.c
2062
2063 int register_kretprobe(struct kretprobe *rp)
2064 {
2065 int ret;
2066 struct kretprobe_instance *inst = NULL;
2067 int i;
2068 void *addr;
2069
2070 ret = kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset);
2071 if (ret)
2072 return ret;
2073
2074 /* If only 'rp->kp.addr' is specified, check reregistering kprobes */
2075 if (rp->kp.addr && warn_kprobe_rereg(&rp->kp))
2076 return -EINVAL;
2077
2078 if (kretprobe_blacklist_size) {
2079 addr = kprobe_addr(&rp->kp);
2080 if (IS_ERR(addr))
2081 return PTR_ERR(addr);
2082
2083 for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
2084 if (kretprobe_blacklist[i].addr == addr)
2085 return -EINVAL;
2086 }
2087 }
2088
2089 rp->kp.pre_handler = pre_handler_kretprobe;
2090 rp->kp.post_handler = NULL;
2091
2092 /* Pre-allocate memory for max kretprobe instances */
2093 if (rp->maxactive <= 0) {
2094 #ifdef CONFIG_PREEMPTION
2095 rp->maxactive = max_t(unsigned int, 10, 2*num_possible_cpus());
2096 #else
2097 rp->maxactive = num_possible_cpus();
2098 #endif
2099 }
2100 rp->freelist.head = NULL;
2101 rp->rph = kzalloc(sizeof(struct kretprobe_holder), GFP_KERNEL);
2102 if (!rp->rph)
2103 return -ENOMEM;
2104
2105 rp->rph->rp = rp;
2106 for (i = 0; i < rp->maxactive; i++) {
> 2107 if (rp->data_size >= 0)
2108 inst = kzalloc(sizeof(struct kretprobe_instance) +
2109 rp->data_size, GFP_KERNEL);
2110 if (inst == NULL) {
2111 refcount_set(&rp->rph->ref, i);
2112 free_rp_inst(rp);
2113 return -ENOMEM;
2114 }
2115 inst->rph = rp->rph;
2116 freelist_add(&inst->freelist, &rp->freelist);
2117 }
2118 refcount_set(&rp->rph->ref, i);
2119
2120 rp->nmissed = 0;
2121 /* Establish function entry probe point */
2122 ret = register_kprobe(&rp->kp);
2123 if (ret != 0)
2124 free_rp_inst(rp);
2125 return ret;
2126 }
2127 EXPORT_SYMBOL_GPL(register_kretprobe);
2128
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] kprobes: fix out-of-bounds in register_kretprobe
2021-12-05 4:26 ` kernel test robot
@ 2021-12-06 0:22 ` Masami Hiramatsu
0 siblings, 0 replies; 6+ messages in thread
From: Masami Hiramatsu @ 2021-12-06 0:22 UTC (permalink / raw)
To: kernel test robot
Cc: zhangyue, naveen.n.rao, anil.s.keshavamurthy, davem, mhiramat,
kbuild-all, linux-kernel
Hi Steve,
Can you revert this patch, because as kernel-test bot says that
this does not change anything. (rp::data_size is unsigned.)
At least it should check the result of
"sizeof(struct kretprobe_instance) + rp->data_size". Moreover,
as I sent before as "kprobes: Limit max data_size of the kretprobe instances"
the data_size must be limited to avoid overflow.
Thank you,
On Sun, 5 Dec 2021 12:26:26 +0800
kernel test robot <lkp@intel.com> wrote:
> Hi zhangyue,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on rostedt-trace/for-next]
> [also build test WARNING on v5.16-rc3 next-20211203]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url: https://github.com/0day-ci/linux/commits/zhangyue/kprobes-fix-out-of-bounds-in-register_kretprobe/20211201-135046
> base: https://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git for-next
> config: i386-randconfig-m021-20211203 (https://download.01.org/0day-ci/archive/20211205/202112051255.NQeIOpp8-lkp@intel.com/config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
>
> smatch warnings:
> kernel/kprobes.c:2107 register_kretprobe() warn: always true condition '(rp->data_size >= 0) => (0-u32max >= 0)'
>
> vim +2107 kernel/kprobes.c
>
> 2062
> 2063 int register_kretprobe(struct kretprobe *rp)
> 2064 {
> 2065 int ret;
> 2066 struct kretprobe_instance *inst = NULL;
> 2067 int i;
> 2068 void *addr;
> 2069
> 2070 ret = kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset);
> 2071 if (ret)
> 2072 return ret;
> 2073
> 2074 /* If only 'rp->kp.addr' is specified, check reregistering kprobes */
> 2075 if (rp->kp.addr && warn_kprobe_rereg(&rp->kp))
> 2076 return -EINVAL;
> 2077
> 2078 if (kretprobe_blacklist_size) {
> 2079 addr = kprobe_addr(&rp->kp);
> 2080 if (IS_ERR(addr))
> 2081 return PTR_ERR(addr);
> 2082
> 2083 for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
> 2084 if (kretprobe_blacklist[i].addr == addr)
> 2085 return -EINVAL;
> 2086 }
> 2087 }
> 2088
> 2089 rp->kp.pre_handler = pre_handler_kretprobe;
> 2090 rp->kp.post_handler = NULL;
> 2091
> 2092 /* Pre-allocate memory for max kretprobe instances */
> 2093 if (rp->maxactive <= 0) {
> 2094 #ifdef CONFIG_PREEMPTION
> 2095 rp->maxactive = max_t(unsigned int, 10, 2*num_possible_cpus());
> 2096 #else
> 2097 rp->maxactive = num_possible_cpus();
> 2098 #endif
> 2099 }
> 2100 rp->freelist.head = NULL;
> 2101 rp->rph = kzalloc(sizeof(struct kretprobe_holder), GFP_KERNEL);
> 2102 if (!rp->rph)
> 2103 return -ENOMEM;
> 2104
> 2105 rp->rph->rp = rp;
> 2106 for (i = 0; i < rp->maxactive; i++) {
> > 2107 if (rp->data_size >= 0)
> 2108 inst = kzalloc(sizeof(struct kretprobe_instance) +
> 2109 rp->data_size, GFP_KERNEL);
> 2110 if (inst == NULL) {
> 2111 refcount_set(&rp->rph->ref, i);
> 2112 free_rp_inst(rp);
> 2113 return -ENOMEM;
> 2114 }
> 2115 inst->rph = rp->rph;
> 2116 freelist_add(&inst->freelist, &rp->freelist);
> 2117 }
> 2118 refcount_set(&rp->rph->ref, i);
> 2119
> 2120 rp->nmissed = 0;
> 2121 /* Establish function entry probe point */
> 2122 ret = register_kprobe(&rp->kp);
> 2123 if (ret != 0)
> 2124 free_rp_inst(rp);
> 2125 return ret;
> 2126 }
> 2127 EXPORT_SYMBOL_GPL(register_kretprobe);
> 2128
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread