[PATCH 2/2] tracing/filters: use strcmp() instead of strncmp()

[PATCH 2/2] tracing/filters: use strcmp() instead of strncmp()

[Li Zefan]

Trace filter is not working normally:

  # echo 'name == et' > tracing/events/irq/irq_handler_entry/filter
  # echo 1 > tracing/events/irq/irq_handler_entry/enable
  # cat trace_pipe
      <idle>-0     [001]  1363.423175: irq_handler_entry: irq=18 handler=eth0
      <idle>-0     [001]  1363.934528: irq_handler_entry: irq=18 handler=eth0
      ...

It's because we pass to trace_define_field() the information of
__str_loc_##item, but not the actual string, so pred->str_len == field->size
== sizeof(unsigned short), thus it always compare at most 2 bytes when
filtering on __string() field.

Since __string() is dynamic size, we are not able to set field->size to
string length. Thus this patch uses strcmp() instead of strncmp().

[ Impact: make filter facility working normally for __string() field ]

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/trace/trace.h               |    1 -
 kernel/trace/trace_events_filter.c |    7 ++-----
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 6e735d4..ec8970b 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -758,7 +758,6 @@ struct filter_pred {
 	filter_pred_fn_t fn;
 	u64 val;
 	char str_val[MAX_FILTER_STR_VAL];
-	int str_len;
 	char *field_name;
 	int offset;
 	int not;
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index a854eed..8362586 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -158,7 +158,7 @@ static int filter_pred_string(struct filter_pred *pred, void *event,
 	char *addr = (char *)(event + pred->offset);
 	int cmp, match;
 
-	cmp = strncmp(addr, pred->str_val, pred->str_len);
+	cmp = strcmp(addr, pred->str_val);
 
 	match = (!cmp) ^ pred->not;
 
@@ -182,7 +182,7 @@ static int filter_pred_strloc(struct filter_pred *pred, void *event,
 	char *addr = (char *)(event + str_loc);
 	int cmp, match;
 
-	cmp = strncmp(addr, pred->str_val, pred->str_len);
+	cmp = strcmp(addr, pred->str_val);
 
 	match = (!cmp) ^ pred->not;
 
@@ -341,7 +341,6 @@ static void filter_clear_pred(struct filter_pred *pred)
 {
 	kfree(pred->field_name);
 	pred->field_name = NULL;
-	pred->str_len = 0;
 }
 
 static int filter_set_pred(struct filter_pred *dest,
@@ -576,7 +575,6 @@ static int filter_add_pred(struct filter_parse_state *ps,
 			fn = filter_pred_string;
 		else
 			fn = filter_pred_strloc;
-		pred->str_len = field->size;
 		if (pred->op == OP_NE)
 			pred->not = 1;
 		return filter_add_pred_fn(ps, call, pred, fn);
@@ -957,7 +955,6 @@ static struct filter_pred *create_pred(int op, char *operand1, char *operand2)
 	}
 
 	strcpy(pred->str_val, operand2);
-	pred->str_len = strlen(operand2);
 
 	pred->op = op;
 
-- 
1.5.4.rc3

Re: [PATCH 2/2] tracing/filters: use strcmp() instead of strncmp()

[Frédéric Weisbecker]

2009/5/29 Li Zefan <lizf@cn.fujitsu.com>:
> Trace filter is not working normally:
>
>  # echo 'name == et' > tracing/events/irq/irq_handler_entry/filter
>  # echo 1 > tracing/events/irq/irq_handler_entry/enable
>  # cat trace_pipe
>      <idle>-0     [001]  1363.423175: irq_handler_entry: irq=18 handler=eth0
>      <idle>-0     [001]  1363.934528: irq_handler_entry: irq=18 handler=eth0
>      ...
>
> It's because we pass to trace_define_field() the information of
> __str_loc_##item, but not the actual string, so pred->str_len == field->size
> == sizeof(unsigned short), thus it always compare at most 2 bytes when
> filtering on __string() field.


Weird, I was about sure I set the size of each string() to FILTER_MAX_STRING (or
something like that).

Anyway this patch looks good but it does more than just fixing the
issue, it removes
the string len boundary security we had with strncmp() for every
string (static and
dynamic size).

The potential side effect that comes along this patch would disappear if
you just turn strncmp into strcmp only in filter_pred_strloc().

If you do that also for fixed size strings, then it should be done in
a second patch,
although I guess turning anything here into strcmp is fine because the
strings given
by the user are always limited in their size. But we never know...


Thanks,
Frederic.


> Since __string() is dynamic size, we are not able to set field->size to
> string length. Thus this patch uses strcmp() instead of strncmp().
>
> [ Impact: make filter facility working normally for __string() field ]
>
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/trace/trace.h               |    1 -
>  kernel/trace/trace_events_filter.c |    7 ++-----
>  2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 6e735d4..ec8970b 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -758,7 +758,6 @@ struct filter_pred {
>        filter_pred_fn_t fn;
>        u64 val;
>        char str_val[MAX_FILTER_STR_VAL];
> -       int str_len;
>        char *field_name;
>        int offset;
>        int not;
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index a854eed..8362586 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -158,7 +158,7 @@ static int filter_pred_string(struct filter_pred *pred, void *event,
>        char *addr = (char *)(event + pred->offset);
>        int cmp, match;
>
> -       cmp = strncmp(addr, pred->str_val, pred->str_len);
> +       cmp = strcmp(addr, pred->str_val);
>
>        match = (!cmp) ^ pred->not;
>
> @@ -182,7 +182,7 @@ static int filter_pred_strloc(struct filter_pred *pred, void *event,
>        char *addr = (char *)(event + str_loc);
>        int cmp, match;
>
> -       cmp = strncmp(addr, pred->str_val, pred->str_len);
> +       cmp = strcmp(addr, pred->str_val);
>
>        match = (!cmp) ^ pred->not;
>
> @@ -341,7 +341,6 @@ static void filter_clear_pred(struct filter_pred *pred)
>  {
>        kfree(pred->field_name);
>        pred->field_name = NULL;
> -       pred->str_len = 0;
>  }
>
>  static int filter_set_pred(struct filter_pred *dest,
> @@ -576,7 +575,6 @@ static int filter_add_pred(struct filter_parse_state *ps,
>                        fn = filter_pred_string;
>                else
>                        fn = filter_pred_strloc;
> -               pred->str_len = field->size;
>                if (pred->op == OP_NE)
>                        pred->not = 1;
>                return filter_add_pred_fn(ps, call, pred, fn);
> @@ -957,7 +955,6 @@ static struct filter_pred *create_pred(int op, char *operand1, char *operand2)
>        }
>
>        strcpy(pred->str_val, operand2);
> -       pred->str_len = strlen(operand2);
>
>        pred->op = op;
>
> --
> 1.5.4.rc3
>
>

Re: [PATCH 2/2] tracing/filters: use strcmp() instead of strncmp()

[Li Zefan]

Frédéric Weisbecker wrote:
> 2009/5/29 Li Zefan <lizf@cn.fujitsu.com>:
>> Trace filter is not working normally:
>>
>>  # echo 'name == et' > tracing/events/irq/irq_handler_entry/filter
>>  # echo 1 > tracing/events/irq/irq_handler_entry/enable
>>  # cat trace_pipe
>>      <idle>-0     [001]  1363.423175: irq_handler_entry: irq=18 handler=eth0
>>      <idle>-0     [001]  1363.934528: irq_handler_entry: irq=18 handler=eth0
>>      ...
>>
>> It's because we pass to trace_define_field() the information of
>> __str_loc_##item, but not the actual string, so pred->str_len == field->size
>> == sizeof(unsigned short), thus it always compare at most 2 bytes when
>> filtering on __string() field.
> 
> 
> Weird, I was about sure I set the size of each string() to FILTER_MAX_STRING (or
> something like that).
> 
> Anyway this patch looks good but it does more than just fixing the
> issue, it removes
> the string len boundary security we had with strncmp() for every
> string (static and
> dynamic size).
> 
> The potential side effect that comes along this patch would disappear if
> you just turn strncmp into strcmp only in filter_pred_strloc().
> 
> If you do that also for fixed size strings, then it should be done in
> a second patch,
> although I guess turning anything here into strcmp is fine because the
> strings given
> by the user are always limited in their size. But we never know...
> 

I don't think there's any security issue. It's irrelevant how big the user-input
strings are. The point is those strings are guaranteed to be NULL-terminated.
Am I missing something?

And I don't think it's necessary to make 2 patches that each patch converts
one strncmp to strcmp. But maybe it's better to improve this changelog?

> 
> Thanks,
> Frederic.
> 
> 
>> Since __string() is dynamic size, we are not able to set field->size to
>> string length. Thus this patch uses strcmp() instead of strncmp().
>>
>> [ Impact: make filter facility working normally for __string() field ]
>>
>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>> ---

Re: [PATCH 2/2] tracing/filters: use strcmp() instead of strncmp()

[Frederic Weisbecker]

On Sat, May 30, 2009 at 05:06:39PM +0800, Li Zefan wrote:
> Frédéric Weisbecker wrote:
> > 2009/5/29 Li Zefan <lizf@cn.fujitsu.com>:
> >> Trace filter is not working normally:
> >>
> >>  # echo 'name == et' > tracing/events/irq/irq_handler_entry/filter
> >>  # echo 1 > tracing/events/irq/irq_handler_entry/enable
> >>  # cat trace_pipe
> >>      <idle>-0     [001]  1363.423175: irq_handler_entry: irq=18 handler=eth0
> >>      <idle>-0     [001]  1363.934528: irq_handler_entry: irq=18 handler=eth0
> >>      ...
> >>
> >> It's because we pass to trace_define_field() the information of
> >> __str_loc_##item, but not the actual string, so pred->str_len == field->size
> >> == sizeof(unsigned short), thus it always compare at most 2 bytes when
> >> filtering on __string() field.
> > 
> > 
> > Weird, I was about sure I set the size of each string() to FILTER_MAX_STRING (or
> > something like that).
> > 
> > Anyway this patch looks good but it does more than just fixing the
> > issue, it removes
> > the string len boundary security we had with strncmp() for every
> > string (static and
> > dynamic size).
> > 
> > The potential side effect that comes along this patch would disappear if
> > you just turn strncmp into strcmp only in filter_pred_strloc().
> > 
> > If you do that also for fixed size strings, then it should be done in
> > a second patch,
> > although I guess turning anything here into strcmp is fine because the
> > strings given
> > by the user are always limited in their size. But we never know...
> > 
> 
> I don't think there's any security issue. It's irrelevant how big the user-input
> strings are. The point is those strings are guaranteed to be NULL-terminated.
> Am I missing something?
> 
> And I don't think it's necessary to make 2 patches that each patch converts
> one strncmp to strcmp. But maybe it's better to improve this changelog?


Hmm, you must be right, indeed they seem to be guaranted beeing NULL-terminated
strings.

But at least improve your changelog to explain why your solution is
safe.

Thanks,
Frederic.

 
> > 
> > Thanks,
> > Frederic.
> > 
> > 
> >> Since __string() is dynamic size, we are not able to set field->size to
> >> string length. Thus this patch uses strcmp() instead of strncmp().
> >>
> >> [ Impact: make filter facility working normally for __string() field ]
> >>
> >> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> >> ---

Re: [PATCH 2/2] tracing/filters: use strcmp() instead of strncmp()

[Li Zefan]

Frederic Weisbecker wrote:
> On Sat, May 30, 2009 at 05:06:39PM +0800, Li Zefan wrote:
>> Frédéric Weisbecker wrote:
>>> 2009/5/29 Li Zefan <lizf@cn.fujitsu.com>:
>>>> Trace filter is not working normally:
>>>>
>>>>  # echo 'name == et' > tracing/events/irq/irq_handler_entry/filter
>>>>  # echo 1 > tracing/events/irq/irq_handler_entry/enable
>>>>  # cat trace_pipe
>>>>      <idle>-0     [001]  1363.423175: irq_handler_entry: irq=18 handler=eth0
>>>>      <idle>-0     [001]  1363.934528: irq_handler_entry: irq=18 handler=eth0
>>>>      ...
>>>>
>>>> It's because we pass to trace_define_field() the information of
>>>> __str_loc_##item, but not the actual string, so pred->str_len == field->size
>>>> == sizeof(unsigned short), thus it always compare at most 2 bytes when
>>>> filtering on __string() field.
>>>
>>> Weird, I was about sure I set the size of each string() to FILTER_MAX_STRING (or
>>> something like that).
>>>
>>> Anyway this patch looks good but it does more than just fixing the
>>> issue, it removes
>>> the string len boundary security we had with strncmp() for every
>>> string (static and
>>> dynamic size).
>>>
>>> The potential side effect that comes along this patch would disappear if
>>> you just turn strncmp into strcmp only in filter_pred_strloc().
>>>
>>> If you do that also for fixed size strings, then it should be done in
>>> a second patch,
>>> although I guess turning anything here into strcmp is fine because the
>>> strings given
>>> by the user are always limited in their size. But we never know...
>>>
>> I don't think there's any security issue. It's irrelevant how big the user-input
>> strings are. The point is those strings are guaranteed to be NULL-terminated.
>> Am I missing something?
>>
>> And I don't think it's necessary to make 2 patches that each patch converts
>> one strncmp to strcmp. But maybe it's better to improve this changelog?
> 
> Hmm, you must be right, indeed they seem to be guaranted beeing NULL-terminated
> strings.
> 

Sorry, I was wrong. :(

Though the user-input strings are guaranted to be NULL-terminated, strings
generated by TRACE_EVENT might not.

We define static strings this way:
	TP_struct(
		__array(char, foo, LEN)
	)
But foo is not necessarily a string, though I doubt someone will use it
as non-string char array.

Dynamic string is fine, because assign_str() makes it NULL-terminated.

So we can use strcmp() for dynamic strings, but we'd better use strncmp() for
static string.

Re: [PATCH 2/2] tracing/filters: use strcmp() instead of strncmp()

[Frederic Weisbecker]

On Sun, May 31, 2009 at 04:27:42PM +0800, Li Zefan wrote:
> Frederic Weisbecker wrote:
> > On Sat, May 30, 2009 at 05:06:39PM +0800, Li Zefan wrote:
> >> Frédéric Weisbecker wrote:
> >>> 2009/5/29 Li Zefan <lizf@cn.fujitsu.com>:
> >>>> Trace filter is not working normally:
> >>>>
> >>>>  # echo 'name == et' > tracing/events/irq/irq_handler_entry/filter
> >>>>  # echo 1 > tracing/events/irq/irq_handler_entry/enable
> >>>>  # cat trace_pipe
> >>>>      <idle>-0     [001]  1363.423175: irq_handler_entry: irq=18 handler=eth0
> >>>>      <idle>-0     [001]  1363.934528: irq_handler_entry: irq=18 handler=eth0
> >>>>      ...
> >>>>
> >>>> It's because we pass to trace_define_field() the information of
> >>>> __str_loc_##item, but not the actual string, so pred->str_len == field->size
> >>>> == sizeof(unsigned short), thus it always compare at most 2 bytes when
> >>>> filtering on __string() field.
> >>>
> >>> Weird, I was about sure I set the size of each string() to FILTER_MAX_STRING (or
> >>> something like that).
> >>>
> >>> Anyway this patch looks good but it does more than just fixing the
> >>> issue, it removes
> >>> the string len boundary security we had with strncmp() for every
> >>> string (static and
> >>> dynamic size).
> >>>
> >>> The potential side effect that comes along this patch would disappear if
> >>> you just turn strncmp into strcmp only in filter_pred_strloc().
> >>>
> >>> If you do that also for fixed size strings, then it should be done in
> >>> a second patch,
> >>> although I guess turning anything here into strcmp is fine because the
> >>> strings given
> >>> by the user are always limited in their size. But we never know...
> >>>
> >> I don't think there's any security issue. It's irrelevant how big the user-input
> >> strings are. The point is those strings are guaranteed to be NULL-terminated.
> >> Am I missing something?
> >>
> >> And I don't think it's necessary to make 2 patches that each patch converts
> >> one strncmp to strcmp. But maybe it's better to improve this changelog?
> > 
> > Hmm, you must be right, indeed they seem to be guaranted beeing NULL-terminated
> > strings.
> > 
> 
> Sorry, I was wrong. :(
> 
> Though the user-input strings are guaranted to be NULL-terminated, strings
> generated by TRACE_EVENT might not.
> 
> We define static strings this way:
> 	TP_struct(
> 		__array(char, foo, LEN)
> 	)
> But foo is not necessarily a string, though I doubt someone will use it
> as non-string char array.


Yeah, but the user defined comparison operand is NULL terminated.
So the strcmp will stop at this boundary.


 
> Dynamic string is fine, because assign_str() makes it NULL-terminated.
> 
> So we can use strcmp() for dynamic strings, but we'd better use strncmp() for
> static string.
> 
> 

Re: [PATCH 2/2] tracing/filters: use strcmp() instead of strncmp()

[Li Zefan]

>>>> I don't think there's any security issue. It's irrelevant how big the user-input
>>>> strings are. The point is those strings are guaranteed to be NULL-terminated.
>>>> Am I missing something?
>>>>
>>>> And I don't think it's necessary to make 2 patches that each patch converts
>>>> one strncmp to strcmp. But maybe it's better to improve this changelog?
>>> Hmm, you must be right, indeed they seem to be guaranted beeing NULL-terminated
>>> strings.
>>>
>> Sorry, I was wrong. :(
>>
>> Though the user-input strings are guaranted to be NULL-terminated, strings
>> generated by TRACE_EVENT might not.
>>
>> We define static strings this way:
>> 	TP_struct(
>> 		__array(char, foo, LEN)
>> 	)
>> But foo is not necessarily a string, though I doubt someone will use it
>> as non-string char array.
> 
> 
> Yeah, but the user defined comparison operand is NULL terminated.
> So the strcmp will stop at this boundary.
> 

The user input string is NULL terminated and is limited to MAX_FILTER_STR_VAL,
and it's strcmp() not strcpy(), but it's still unsafe. No?

	cmp = strcmp(addr, pred->str_val);

If addr is not NULL-terminated string but char array, and length of
str_val > length of addr, then we'll be exceeding the boundary of the
array.

> 
>  
>> Dynamic string is fine, because assign_str() makes it NULL-terminated.
>>
>> So we can use strcmp() for dynamic strings, but we'd better use strncmp() for
>> static string.
>>
>>
> 
> 
> 

Re: [PATCH 2/2] tracing/filters: use strcmp() instead of strncmp()

[Frederic Weisbecker]

On Mon, Jun 01, 2009 at 01:45:47PM +0800, Li Zefan wrote:
> >>>> I don't think there's any security issue. It's irrelevant how big the user-input
> >>>> strings are. The point is those strings are guaranteed to be NULL-terminated.
> >>>> Am I missing something?
> >>>>
> >>>> And I don't think it's necessary to make 2 patches that each patch converts
> >>>> one strncmp to strcmp. But maybe it's better to improve this changelog?
> >>> Hmm, you must be right, indeed they seem to be guaranted beeing NULL-terminated
> >>> strings.
> >>>
> >> Sorry, I was wrong. :(
> >>
> >> Though the user-input strings are guaranted to be NULL-terminated, strings
> >> generated by TRACE_EVENT might not.
> >>
> >> We define static strings this way:
> >> 	TP_struct(
> >> 		__array(char, foo, LEN)
> >> 	)
> >> But foo is not necessarily a string, though I doubt someone will use it
> >> as non-string char array.
> > 
> > 
> > Yeah, but the user defined comparison operand is NULL terminated.
> > So the strcmp will stop at this boundary.
> > 
> 
> The user input string is NULL terminated and is limited to MAX_FILTER_STR_VAL,
> and it's strcmp() not strcpy(), but it's still unsafe. No?
> 
> 	cmp = strcmp(addr, pred->str_val);
> 
> If addr is not NULL-terminated string but char array, and length of
> str_val > length of addr, then we'll be exceeding the boundary of the
> array.



No, once both strings appear to be different, strcmp returns.
As an example, the generic strcmp in lib/string.c is as follows:

int strcmp(const char *cs, const char *ct)
{
	signed char __res;

	while (1) {
		if ((__res = *cs - *ct++) != 0 || !*cs++)
			break;
	}
	return __res;
}

Once cs[n] != ct[n], or !cs[n] || !ct[n], strcmp() stops,
and the x86 implementation does exactly the same.

So I guess it's safe.

 
> > 
> >  
> >> Dynamic string is fine, because assign_str() makes it NULL-terminated.
> >>
> >> So we can use strcmp() for dynamic strings, but we'd better use strncmp() for
> >> static string.
> >>
> >>
> > 
> > 
> > 

Re: [PATCH 2/2] tracing/filters: use strcmp() instead of strncmp()

[Li Zefan]

Frederic Weisbecker wrote:
> On Mon, Jun 01, 2009 at 01:45:47PM +0800, Li Zefan wrote:
>>>>>> I don't think there's any security issue. It's irrelevant how big the user-input
>>>>>> strings are. The point is those strings are guaranteed to be NULL-terminated.
>>>>>> Am I missing something?
>>>>>>
>>>>>> And I don't think it's necessary to make 2 patches that each patch converts
>>>>>> one strncmp to strcmp. But maybe it's better to improve this changelog?
>>>>> Hmm, you must be right, indeed they seem to be guaranted beeing NULL-terminated
>>>>> strings.
>>>>>
>>>> Sorry, I was wrong. :(
>>>>
>>>> Though the user-input strings are guaranted to be NULL-terminated, strings
>>>> generated by TRACE_EVENT might not.
>>>>
>>>> We define static strings this way:
>>>> 	TP_struct(
>>>> 		__array(char, foo, LEN)
>>>> 	)
>>>> But foo is not necessarily a string, though I doubt someone will use it
>>>> as non-string char array.
>>>
>>> Yeah, but the user defined comparison operand is NULL terminated.
>>> So the strcmp will stop at this boundary.
>>>
>> The user input string is NULL terminated and is limited to MAX_FILTER_STR_VAL,
>> and it's strcmp() not strcpy(), but it's still unsafe. No?
>>
>> 	cmp = strcmp(addr, pred->str_val);
>>
>> If addr is not NULL-terminated string but char array, and length of
>> str_val > length of addr, then we'll be exceeding the boundary of the
>> array.
> 
> 
> 
> No, once both strings appear to be different, strcmp returns.
> As an example, the generic strcmp in lib/string.c is as follows:
> 
> int strcmp(const char *cs, const char *ct)
> {
> 	signed char __res;
> 
> 	while (1) {
> 		if ((__res = *cs - *ct++) != 0 || !*cs++)
> 			break;
> 	}
> 	return __res;
> }
> 
> Once cs[n] != ct[n], or !cs[n] || !ct[n], strcmp() stops,
> and the x86 implementation does exactly the same.
> 
> So I guess it's safe.
> 

See this example:

cmp = strcmp(addr, pred->str_val);

length(addr) == 6, strlen(str_val) == 10

         123456
addr:    abcdef?
               ^
               |
               v
str_val: abcdefzzzz\0

or the 2 happen to match even after addr overflowed:

         123456
addr:    abcdefzzzz?
                   ^
                   |
                   v
str_val: abcdefzzzz\0

Re: [PATCH 2/2] tracing/filters: use strcmp() instead of strncmp()

[Steven Rostedt]

I recently had some hardware issues, so my mail is all over the place.
I'm going through old email, and trying to clean up my mbox.

On Tue, 2009-06-02 at 08:55 +0800, Li Zefan wrote:
> Frederic Weisbecker wrote:
> > On Mon, Jun 01, 2009 at 01:45:47PM +0800, Li Zefan wrote:
> >>>>>> I don't think there's any security issue. It's irrelevant how big the user-input
> >>>>>> strings are. The point is those strings are guaranteed to be NULL-terminated.
> >>>>>> Am I missing something?
> >>>>>>
> >>>>>> And I don't think it's necessary to make 2 patches that each patch converts
> >>>>>> one strncmp to strcmp. But maybe it's better to improve this changelog?
> >>>>> Hmm, you must be right, indeed they seem to be guaranted beeing NULL-terminated
> >>>>> strings.
> >>>>>
> >>>> Sorry, I was wrong. :(
> >>>>
> >>>> Though the user-input strings are guaranted to be NULL-terminated, strings
> >>>> generated by TRACE_EVENT might not.
> >>>>
> >>>> We define static strings this way:
> >>>> 	TP_struct(
> >>>> 		__array(char, foo, LEN)
> >>>> 	)
> >>>> But foo is not necessarily a string, though I doubt someone will use it
> >>>> as non-string char array.
> >>>
> >>> Yeah, but the user defined comparison operand is NULL terminated.
> >>> So the strcmp will stop at this boundary.
> >>>
> >> The user input string is NULL terminated and is limited to MAX_FILTER_STR_VAL,
> >> and it's strcmp() not strcpy(), but it's still unsafe. No?
> >>
> >> 	cmp = strcmp(addr, pred->str_val);
> >>
> >> If addr is not NULL-terminated string but char array, and length of
> >> str_val > length of addr, then we'll be exceeding the boundary of the
> >> array.
> > 
> > 
> > 
> > No, once both strings appear to be different, strcmp returns.
> > As an example, the generic strcmp in lib/string.c is as follows:
> > 
> > int strcmp(const char *cs, const char *ct)
> > {
> > 	signed char __res;
> > 
> > 	while (1) {
> > 		if ((__res = *cs - *ct++) != 0 || !*cs++)
> > 			break;
> > 	}
> > 	return __res;
> > }
> > 
> > Once cs[n] != ct[n], or !cs[n] || !ct[n], strcmp() stops,
> > and the x86 implementation does exactly the same.
> > 
> > So I guess it's safe.
> > 
> 
> See this example:
> 
> cmp = strcmp(addr, pred->str_val);
> 
> length(addr) == 6, strlen(str_val) == 10
> 
>          123456
> addr:    abcdef?
>                ^
>                |
>                v
> str_val: abcdefzzzz\0
> 
> or the 2 happen to match even after addr overflowed:
> 
>          123456
> addr:    abcdefzzzz?
>                    ^
>                    |
>                    v
> str_val: abcdefzzzz\0
> 

Not sure this is an issue. I may be a little out of context here, but
isn't addr coming from the event? The event is made in the kernel and
should be fine?

What ever the case, the bug you originally mentioned is still there (I
just tried it out on the latest tip). That is, name == et will match
"eth0".

-- Steve

Re: [PATCH 2/2] tracing/filters: use strcmp() instead of strncmp()

[Li Zefan]

> Not sure this is an issue. I may be a little out of context here, but
> isn't addr coming from the event? The event is made in the kernel and
> should be fine?
> 
> What ever the case, the bug you originally mentioned is still there (I
> just tried it out on the latest tip). That is, name == et will match
> "eth0".
> 

Strange. I fixed it with this commit:

========
commit 7d536cb3fb9993bdcd5a2fbaa6b0670ded4e101c
Author: Li Zefan <lizf@cn.fujitsu.com>
Date:   Thu Jul 16 10:54:02 2009 +0800

    tracing/events: record the size of dynamic arrays

    When a dynamic array is defined, we add __data_loc_foo in
    trace_entry to record the offset of the array, but the
    size of the array is not recorded, which causes 2 problems:

    - the event filter just compares the first 2 chars of the strings. <-- note here!!

    - parsers can't parse dynamic arrays.

    So we encode the size of each dynamic array in the higher 16 bits
    of __data_loc_foo, while the offset is in lower 16 bits.
========

And I just double checked it to confirm that the bug has been fixed.

Re: [PATCH 2/2] tracing/filters: use strcmp() instead of strncmp()

[Steven Rostedt]

On Wed, 2009-09-09 at 09:21 +0800, Li Zefan wrote:
> > Not sure this is an issue. I may be a little out of context here, but
> > isn't addr coming from the event? The event is made in the kernel and
> > should be fine?
> > 
> > What ever the case, the bug you originally mentioned is still there (I
> > just tried it out on the latest tip). That is, name == et will match
> > "eth0".
> > 
> 
> Strange. I fixed it with this commit:
> 
> ========
> commit 7d536cb3fb9993bdcd5a2fbaa6b0670ded4e101c
> Author: Li Zefan <lizf@cn.fujitsu.com>
> Date:   Thu Jul 16 10:54:02 2009 +0800
> 
>     tracing/events: record the size of dynamic arrays

My fault, I booted into the wrong (older) kernel when I did my test.

Sorry for the noise.

-- Steve