From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755010AbZEaN3M (ORCPT ); Sun, 31 May 2009 09:29:12 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1754674AbZEaN27 (ORCPT ); Sun, 31 May 2009 09:28:59 -0400 Received: from mail-ew0-f176.google.com ([209.85.219.176]:46143 "EHLO mail-ew0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754621AbZEaN26 (ORCPT ); Sun, 31 May 2009 09:28:58 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:content-transfer-encoding :in-reply-to:user-agent; b=V10xEV5hkb8Re2qStjYF26NnORsVyOxI4CSsPJOi/GYm3qFhC8/ja3JNyj+7mSBloQ NpUfCeACUm7QtjTamgzfrqnsmy3ZmTdZOg0mA4vQ5JF8GSLcxL8uZWRPmD/GfvsCl0ae /chPzNXR4+HXzm7YqA4+/NYeR3TC++iITL3H8= Date: Sun, 31 May 2009 15:28:56 +0200 From: Frederic Weisbecker To: Li Zefan Cc: Steven Rostedt , Tom Zanussi , Ingo Molnar , LKML Subject: Re: [PATCH 2/2] tracing/filters: use strcmp() instead of strncmp() Message-ID: <20090531132853.GA6013@nowhere> References: <4A1F9FAC.6020506@cn.fujitsu.com> <4A20F71F.6030703@cn.fujitsu.com> <20090530135236.GB5969@nowhere> <4A223F7E.3090203@cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <4A223F7E.3090203@cn.fujitsu.com> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 : > >>>> 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 > >>>> -0 [001] 1363.423175: irq_handler_entry: irq=18 handler=eth0 > >>>> -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. > >