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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/