linux-trace-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [bug report] tracing: probeevent: Add array type support
@ 2023-06-26 14:33 Dan Carpenter
  2023-06-28  0:09 ` Masami Hiramatsu
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Dan Carpenter @ 2023-06-26 14:33 UTC (permalink / raw)
  To: mhiramat; +Cc: linux-trace-kernel

Hello Masami Hiramatsu,

The patch 40b53b771806: "tracing: probeevent: Add array type support"
from Apr 25, 2018, leads to the following Smatch static checker
warning:

	kernel/trace/trace_probe_tmpl.h:207 process_fetch_insn_bottom()
	warn: using error codes for math 'ret'

kernel/trace/trace_probe_tmpl.h
    122 static nokprobe_inline int
    123 process_fetch_insn_bottom(struct fetch_insn *code, unsigned long val,
    124                            void *dest, void *base)
    125 {
    126         struct fetch_insn *s3 = NULL;
    127         int total = 0, ret = 0, i = 0;
    128         u32 loc = 0;
    129         unsigned long lval = val;
    130 
    131 stage2:
    132         /* 2nd stage: dereference memory if needed */
    133         do {
    134                 if (code->op == FETCH_OP_DEREF) {
    135                         lval = val;
    136                         ret = probe_mem_read(&val, (void *)val + code->offset,
    137                                              sizeof(val));
    138                 } else if (code->op == FETCH_OP_UDEREF) {
    139                         lval = val;
    140                         ret = probe_mem_read_user(&val,
    141                                  (void *)val + code->offset, sizeof(val));
    142                 } else
    143                         break;
    144                 if (ret)
    145                         return ret;
    146                 code++;
    147         } while (1);
    148 
    149         s3 = code;
    150 stage3:
    151         /* 3rd stage: store value to buffer */
    152         if (unlikely(!dest)) {
    153                 switch (code->op) {
    154                 case FETCH_OP_ST_STRING:
    155                         ret = fetch_store_strlen(val + code->offset);
    156                         code++;
    157                         goto array;
    158                 case FETCH_OP_ST_USTRING:
    159                         ret += fetch_store_strlen_user(val + code->offset);
    160                         code++;
    161                         goto array;
    162                 case FETCH_OP_ST_SYMSTR:
    163                         ret += fetch_store_symstrlen(val + code->offset);

I think all these functions return zero on error.

    164                         code++;
    165                         goto array;
    166                 default:
    167                         return -EILSEQ;
    168                 }
    169         }
    170 
    171         switch (code->op) {
    172         case FETCH_OP_ST_RAW:
    173                 fetch_store_raw(val, code, dest);
    174                 break;
    175         case FETCH_OP_ST_MEM:
    176                 probe_mem_read(dest, (void *)val + code->offset, code->size);
    177                 break;
    178         case FETCH_OP_ST_UMEM:
    179                 probe_mem_read_user(dest, (void *)val + code->offset, code->size);
    180                 break;
    181         case FETCH_OP_ST_STRING:
    182                 loc = *(u32 *)dest;
    183                 ret = fetch_store_string(val + code->offset, dest, base);

This function return -EFAULT if copy_from_user() fails.

    184                 break;
    185         case FETCH_OP_ST_USTRING:
    186                 loc = *(u32 *)dest;
    187                 ret = fetch_store_string_user(val + code->offset, dest, base);

Same.

    188                 break;
    189         case FETCH_OP_ST_SYMSTR:
    190                 loc = *(u32 *)dest;
    191                 ret = fetch_store_symstring(val + code->offset, dest, base);

My guess is that Smatch thinks this returns -ENOMEM.

    192                 break;
    193         default:
    194                 return -EILSEQ;
    195         }
    196         code++;
    197 
    198         /* 4th stage: modify stored value if needed */
    199         if (code->op == FETCH_OP_MOD_BF) {
    200                 fetch_apply_bitfield(code, dest);
    201                 code++;
    202         }
    203 
    204 array:
    205         /* the last stage: Loop on array */
    206         if (code->op == FETCH_OP_LP_ARRAY) {
--> 207                 total += ret;

This is an unpublished check because I need to go through and make a
list of all the functions which can't fail in real life.  But the rule
here is that Smatch doesn't like adding error codes to anything.

    208                 if (++i < code->param) {
    209                         code = s3;
    210                         if (s3->op != FETCH_OP_ST_STRING &&
    211                             s3->op != FETCH_OP_ST_USTRING) {
    212                                 dest += s3->size;
    213                                 val += s3->size;
    214                                 goto stage3;
    215                         }
    216                         code--;
    217                         val = lval + sizeof(char *);
    218                         if (dest) {
    219                                 dest += sizeof(u32);
    220                                 *(u32 *)dest = update_data_loc(loc, ret);
    221                         }
    222                         goto stage2;
    223                 }
    224                 code++;
    225                 ret = total;
    226         }
    227 
    228         return code->op == FETCH_OP_END ? ret : -EILSEQ;
    229 }

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [bug report] tracing: probeevent: Add array type support
  2023-06-26 14:33 [bug report] tracing: probeevent: Add array type support Dan Carpenter
@ 2023-06-28  0:09 ` Masami Hiramatsu
  2023-06-29 14:13 ` [PATCH 1/2] tracing/probes: Fix to avoid double count of the string length on the array Masami Hiramatsu (Google)
  2023-06-29 14:13 ` [PATCH 2/2] tracing/probes: Fix to exit fetching if an error is detected Masami Hiramatsu (Google)
  2 siblings, 0 replies; 8+ messages in thread
From: Masami Hiramatsu @ 2023-06-28  0:09 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: linux-trace-kernel

On Mon, 26 Jun 2023 17:33:21 +0300
Dan Carpenter <dan.carpenter@linaro.org> wrote:

> Hello Masami Hiramatsu,
> 
> The patch 40b53b771806: "tracing: probeevent: Add array type support"
> from Apr 25, 2018, leads to the following Smatch static checker
> warning:
> 
> 	kernel/trace/trace_probe_tmpl.h:207 process_fetch_insn_bottom()
> 	warn: using error codes for math 'ret'
> 
> kernel/trace/trace_probe_tmpl.h
>     122 static nokprobe_inline int
>     123 process_fetch_insn_bottom(struct fetch_insn *code, unsigned long val,
>     124                            void *dest, void *base)
>     125 {
>     126         struct fetch_insn *s3 = NULL;
>     127         int total = 0, ret = 0, i = 0;
>     128         u32 loc = 0;
>     129         unsigned long lval = val;
>     130 
>     131 stage2:
>     132         /* 2nd stage: dereference memory if needed */
>     133         do {
>     134                 if (code->op == FETCH_OP_DEREF) {
>     135                         lval = val;
>     136                         ret = probe_mem_read(&val, (void *)val + code->offset,
>     137                                              sizeof(val));
>     138                 } else if (code->op == FETCH_OP_UDEREF) {
>     139                         lval = val;
>     140                         ret = probe_mem_read_user(&val,
>     141                                  (void *)val + code->offset, sizeof(val));
>     142                 } else
>     143                         break;
>     144                 if (ret)
>     145                         return ret;
>     146                 code++;
>     147         } while (1);
>     148 
>     149         s3 = code;
>     150 stage3:
>     151         /* 3rd stage: store value to buffer */
>     152         if (unlikely(!dest)) {
>     153                 switch (code->op) {
>     154                 case FETCH_OP_ST_STRING:
>     155                         ret = fetch_store_strlen(val + code->offset);
>     156                         code++;
>     157                         goto array;
>     158                 case FETCH_OP_ST_USTRING:
>     159                         ret += fetch_store_strlen_user(val + code->offset);
>     160                         code++;
>     161                         goto array;
>     162                 case FETCH_OP_ST_SYMSTR:
>     163                         ret += fetch_store_symstrlen(val + code->offset);
> 
> I think all these functions return zero on error.
> 
>     164                         code++;
>     165                         goto array;
>     166                 default:
>     167                         return -EILSEQ;
>     168                 }
>     169         }
>     170 
>     171         switch (code->op) {
>     172         case FETCH_OP_ST_RAW:
>     173                 fetch_store_raw(val, code, dest);
>     174                 break;
>     175         case FETCH_OP_ST_MEM:
>     176                 probe_mem_read(dest, (void *)val + code->offset, code->size);
>     177                 break;
>     178         case FETCH_OP_ST_UMEM:
>     179                 probe_mem_read_user(dest, (void *)val + code->offset, code->size);
>     180                 break;
>     181         case FETCH_OP_ST_STRING:
>     182                 loc = *(u32 *)dest;
>     183                 ret = fetch_store_string(val + code->offset, dest, base);
> 
> This function return -EFAULT if copy_from_user() fails.
> 
>     184                 break;
>     185         case FETCH_OP_ST_USTRING:
>     186                 loc = *(u32 *)dest;
>     187                 ret = fetch_store_string_user(val + code->offset, dest, base);
> 
> Same.
> 
>     188                 break;
>     189         case FETCH_OP_ST_SYMSTR:
>     190                 loc = *(u32 *)dest;
>     191                 ret = fetch_store_symstring(val + code->offset, dest, base);
> 
> My guess is that Smatch thinks this returns -ENOMEM.
> 
>     192                 break;
>     193         default:
>     194                 return -EILSEQ;
>     195         }
>     196         code++;
>     197 
>     198         /* 4th stage: modify stored value if needed */
>     199         if (code->op == FETCH_OP_MOD_BF) {
>     200                 fetch_apply_bitfield(code, dest);
>     201                 code++;
>     202         }
>     203 
>     204 array:
>     205         /* the last stage: Loop on array */
>     206         if (code->op == FETCH_OP_LP_ARRAY) {
> --> 207                 total += ret;
> 
> This is an unpublished check because I need to go through and make a
> list of all the functions which can't fail in real life.  But the rule
> here is that Smatch doesn't like adding error codes to anything.

Good catch! Yes, that is a problem. Let me fix that.

Thanks!

> 
>     208                 if (++i < code->param) {
>     209                         code = s3;
>     210                         if (s3->op != FETCH_OP_ST_STRING &&
>     211                             s3->op != FETCH_OP_ST_USTRING) {
>     212                                 dest += s3->size;
>     213                                 val += s3->size;
>     214                                 goto stage3;
>     215                         }
>     216                         code--;
>     217                         val = lval + sizeof(char *);
>     218                         if (dest) {
>     219                                 dest += sizeof(u32);
>     220                                 *(u32 *)dest = update_data_loc(loc, ret);
>     221                         }
>     222                         goto stage2;
>     223                 }
>     224                 code++;
>     225                 ret = total;
>     226         }
>     227 
>     228         return code->op == FETCH_OP_END ? ret : -EILSEQ;
>     229 }
> 
> regards,
> dan carpenter


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] tracing/probes: Fix to avoid double count of the string length on the array
  2023-06-26 14:33 [bug report] tracing: probeevent: Add array type support Dan Carpenter
  2023-06-28  0:09 ` Masami Hiramatsu
@ 2023-06-29 14:13 ` Masami Hiramatsu (Google)
  2023-07-02  1:08   ` Steven Rostedt
  2023-06-29 14:13 ` [PATCH 2/2] tracing/probes: Fix to exit fetching if an error is detected Masami Hiramatsu (Google)
  2 siblings, 1 reply; 8+ messages in thread
From: Masami Hiramatsu (Google) @ 2023-06-29 14:13 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Dan Carpenter, linux-trace-kernel, LKML, Masami Hiramatsu

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

If there is an array is specified with the ustring or symstr, the length of
the strings are accumlated on both of 'ret' and 'total', which means the
length is double counted.
Just set the length to the 'ret' value to aviud double count.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/8819b154-2ba1-43c3-98a2-cbde20892023@moroto.mountain/
Fixes: 88903c464321 ("tracing/probe: Add ustring type for user-space string")
Cc: stable@vger.kernel.org
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_probe_tmpl.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_probe_tmpl.h b/kernel/trace/trace_probe_tmpl.h
index 00707630788d..4735c5cb76fa 100644
--- a/kernel/trace/trace_probe_tmpl.h
+++ b/kernel/trace/trace_probe_tmpl.h
@@ -156,11 +156,11 @@ process_fetch_insn_bottom(struct fetch_insn *code, unsigned long val,
 			code++;
 			goto array;
 		case FETCH_OP_ST_USTRING:
-			ret += fetch_store_strlen_user(val + code->offset);
+			ret = fetch_store_strlen_user(val + code->offset);
 			code++;
 			goto array;
 		case FETCH_OP_ST_SYMSTR:
-			ret += fetch_store_symstrlen(val + code->offset);
+			ret = fetch_store_symstrlen(val + code->offset);
 			code++;
 			goto array;
 		default:


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] tracing/probes: Fix to exit fetching if an error is detected
  2023-06-26 14:33 [bug report] tracing: probeevent: Add array type support Dan Carpenter
  2023-06-28  0:09 ` Masami Hiramatsu
  2023-06-29 14:13 ` [PATCH 1/2] tracing/probes: Fix to avoid double count of the string length on the array Masami Hiramatsu (Google)
@ 2023-06-29 14:13 ` Masami Hiramatsu (Google)
  2023-07-02  1:16   ` Steven Rostedt
  2 siblings, 1 reply; 8+ messages in thread
From: Masami Hiramatsu (Google) @ 2023-06-29 14:13 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Dan Carpenter, linux-trace-kernel, LKML, Masami Hiramatsu

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Fix to exit fetching arguments if an error is detected when storing
strings. Without this fix, if an array is specified with string types
it may point wrong address to store the data.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/8819b154-2ba1-43c3-98a2-cbde20892023@moroto.mountain/
Fixes: 9b960a38835f ("tracing: probeevent: Unify fetch_insn processing common part")
Cc: stable@vger.kernel.org
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_probe_tmpl.h |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/trace/trace_probe_tmpl.h b/kernel/trace/trace_probe_tmpl.h
index 4735c5cb76fa..d6f2bf69f9bc 100644
--- a/kernel/trace/trace_probe_tmpl.h
+++ b/kernel/trace/trace_probe_tmpl.h
@@ -193,6 +193,8 @@ process_fetch_insn_bottom(struct fetch_insn *code, unsigned long val,
 	default:
 		return -EILSEQ;
 	}
+	if (ret < 0)
+		return ret;
 	code++;
 
 	/* 4th stage: modify stored value if needed */


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] tracing/probes: Fix to avoid double count of the string length on the array
  2023-06-29 14:13 ` [PATCH 1/2] tracing/probes: Fix to avoid double count of the string length on the array Masami Hiramatsu (Google)
@ 2023-07-02  1:08   ` Steven Rostedt
  2023-07-02 12:56     ` Masami Hiramatsu
  0 siblings, 1 reply; 8+ messages in thread
From: Steven Rostedt @ 2023-07-02  1:08 UTC (permalink / raw)
  To: Masami Hiramatsu (Google); +Cc: Dan Carpenter, linux-trace-kernel, LKML

On Thu, 29 Jun 2023 23:13:37 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> If there is an array is specified with the ustring or symstr, the length of

 "If there is an array specified with ustring or .." or "If an array is specified with ustring"

I prefer the latter.

> the strings are accumlated on both of 'ret' and 'total', which means the
> length is double counted.
> Just set the length to the 'ret' value to aviud double count.

					"avoid"

> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/8819b154-2ba1-43c3-98a2-cbde20892023@moroto.mountain/
> Fixes: 88903c464321 ("tracing/probe: Add ustring type for user-space string")
> Cc: stable@vger.kernel.org
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_probe_tmpl.h |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_probe_tmpl.h b/kernel/trace/trace_probe_tmpl.h
> index 00707630788d..4735c5cb76fa 100644
> --- a/kernel/trace/trace_probe_tmpl.h
> +++ b/kernel/trace/trace_probe_tmpl.h
> @@ -156,11 +156,11 @@ process_fetch_insn_bottom(struct fetch_insn *code, unsigned long val,
>  			code++;
>  			goto array;
>  		case FETCH_OP_ST_USTRING:
> -			ret += fetch_store_strlen_user(val + code->offset);
> +			ret = fetch_store_strlen_user(val + code->offset);
>  			code++;
>  			goto array;
>  		case FETCH_OP_ST_SYMSTR:
> -			ret += fetch_store_symstrlen(val + code->offset);
> +			ret = fetch_store_symstrlen(val + code->offset);

Other than the above,

Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve


>  			code++;
>  			goto array;
>  		default:


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] tracing/probes: Fix to exit fetching if an error is detected
  2023-06-29 14:13 ` [PATCH 2/2] tracing/probes: Fix to exit fetching if an error is detected Masami Hiramatsu (Google)
@ 2023-07-02  1:16   ` Steven Rostedt
  2023-07-02 13:46     ` Masami Hiramatsu
  0 siblings, 1 reply; 8+ messages in thread
From: Steven Rostedt @ 2023-07-02  1:16 UTC (permalink / raw)
  To: Masami Hiramatsu (Google); +Cc: Dan Carpenter, linux-trace-kernel, LKML

On Thu, 29 Jun 2023 23:13:46 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Fix to exit fetching arguments if an error is detected when storing
> strings. Without this fix, if an array is specified with string types
> it may point wrong address to store the data.

  "it may store that data at the wrong address".

> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/8819b154-2ba1-43c3-98a2-cbde20892023@moroto.mountain/
> Fixes: 9b960a38835f ("tracing: probeevent: Unify fetch_insn processing common part")
> Cc: stable@vger.kernel.org
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_probe_tmpl.h |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/trace/trace_probe_tmpl.h b/kernel/trace/trace_probe_tmpl.h
> index 4735c5cb76fa..d6f2bf69f9bc 100644
> --- a/kernel/trace/trace_probe_tmpl.h
> +++ b/kernel/trace/trace_probe_tmpl.h
> @@ -193,6 +193,8 @@ process_fetch_insn_bottom(struct fetch_insn *code, unsigned long val,
>  	default:
>  		return -EILSEQ;
>  	}
> +	if (ret < 0)
> +		return ret;

Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve

>  	code++;
>  
>  	/* 4th stage: modify stored value if needed */


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] tracing/probes: Fix to avoid double count of the string length on the array
  2023-07-02  1:08   ` Steven Rostedt
@ 2023-07-02 12:56     ` Masami Hiramatsu
  0 siblings, 0 replies; 8+ messages in thread
From: Masami Hiramatsu @ 2023-07-02 12:56 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Dan Carpenter, linux-trace-kernel, LKML

On Sat, 1 Jul 2023 21:08:40 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 29 Jun 2023 23:13:37 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > If there is an array is specified with the ustring or symstr, the length of
> 
>  "If there is an array specified with ustring or .." or "If an array is specified with ustring"
> 
> I prefer the latter.
> 
> > the strings are accumlated on both of 'ret' and 'total', which means the
> > length is double counted.
> > Just set the length to the 'ret' value to aviud double count.
> 
> 					"avoid"
> 
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/all/8819b154-2ba1-43c3-98a2-cbde20892023@moroto.mountain/
> > Fixes: 88903c464321 ("tracing/probe: Add ustring type for user-space string")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  kernel/trace/trace_probe_tmpl.h |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_probe_tmpl.h b/kernel/trace/trace_probe_tmpl.h
> > index 00707630788d..4735c5cb76fa 100644
> > --- a/kernel/trace/trace_probe_tmpl.h
> > +++ b/kernel/trace/trace_probe_tmpl.h
> > @@ -156,11 +156,11 @@ process_fetch_insn_bottom(struct fetch_insn *code, unsigned long val,
> >  			code++;
> >  			goto array;
> >  		case FETCH_OP_ST_USTRING:
> > -			ret += fetch_store_strlen_user(val + code->offset);
> > +			ret = fetch_store_strlen_user(val + code->offset);
> >  			code++;
> >  			goto array;
> >  		case FETCH_OP_ST_SYMSTR:
> > -			ret += fetch_store_symstrlen(val + code->offset);
> > +			ret = fetch_store_symstrlen(val + code->offset);
> 
> Other than the above,
> 
> Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Thanks! I'll resend it with fixes.

> 
> -- Steve
> 
> 
> >  			code++;
> >  			goto array;
> >  		default:
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] tracing/probes: Fix to exit fetching if an error is detected
  2023-07-02  1:16   ` Steven Rostedt
@ 2023-07-02 13:46     ` Masami Hiramatsu
  0 siblings, 0 replies; 8+ messages in thread
From: Masami Hiramatsu @ 2023-07-02 13:46 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Dan Carpenter, linux-trace-kernel, LKML

On Sat, 1 Jul 2023 21:16:17 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 29 Jun 2023 23:13:46 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Fix to exit fetching arguments if an error is detected when storing
> > strings. Without this fix, if an array is specified with string types
> > it may point wrong address to store the data.
> 
>   "it may store that data at the wrong address".

Good catch! Yes, while updating the data location, the remaining length
and offset becomes wrong and next array element (string) will be stored
in wrong memory address.

> 
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/all/8819b154-2ba1-43c3-98a2-cbde20892023@moroto.mountain/
> > Fixes: 9b960a38835f ("tracing: probeevent: Unify fetch_insn processing common part")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  kernel/trace/trace_probe_tmpl.h |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/kernel/trace/trace_probe_tmpl.h b/kernel/trace/trace_probe_tmpl.h
> > index 4735c5cb76fa..d6f2bf69f9bc 100644
> > --- a/kernel/trace/trace_probe_tmpl.h
> > +++ b/kernel/trace/trace_probe_tmpl.h
> > @@ -193,6 +193,8 @@ process_fetch_insn_bottom(struct fetch_insn *code, unsigned long val,
> >  	default:
> >  		return -EILSEQ;
> >  	}
> > +	if (ret < 0)
> > +		return ret;
> 
> Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Thanks!

> 
> -- Steve
> 
> >  	code++;
> >  
> >  	/* 4th stage: modify stored value if needed */
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-07-02 13:47 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-26 14:33 [bug report] tracing: probeevent: Add array type support Dan Carpenter
2023-06-28  0:09 ` Masami Hiramatsu
2023-06-29 14:13 ` [PATCH 1/2] tracing/probes: Fix to avoid double count of the string length on the array Masami Hiramatsu (Google)
2023-07-02  1:08   ` Steven Rostedt
2023-07-02 12:56     ` Masami Hiramatsu
2023-06-29 14:13 ` [PATCH 2/2] tracing/probes: Fix to exit fetching if an error is detected Masami Hiramatsu (Google)
2023-07-02  1:16   ` Steven Rostedt
2023-07-02 13:46     ` Masami Hiramatsu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).