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

[Frederic Weisbecker <fweisbec@gmail.com>]

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.
> >>
> >>
> > 
> > 
> >