From owner-freebsd-new-bus@FreeBSD.ORG Fri Nov 6 15:51:04 2009 Return-Path: Delivered-To: freebsd-new-bus@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 9C7211065672; Fri, 6 Nov 2009 15:51:04 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 6CA5E8FC1D; Fri, 6 Nov 2009 15:51:04 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id 064A546B1A; Fri, 6 Nov 2009 10:51:04 -0500 (EST) Received: from jhbbsd.hudson-trading.com (unknown [209.249.190.8]) by bigwig.baldwin.cx (Postfix) with ESMTPA id 2F36E8A01D; Fri, 6 Nov 2009 10:51:03 -0500 (EST) From: John Baldwin To: Attilio Rao Date: Fri, 6 Nov 2009 10:43:52 -0500 User-Agent: KMail/1.9.7 References: <3bbf2fe10911060720m6d6919ffw91dcc5b6c1c2016a@mail.gmail.com> In-Reply-To: <3bbf2fe10911060720m6d6919ffw91dcc5b6c1c2016a@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200911061043.52738.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.0.1 (bigwig.baldwin.cx); Fri, 06 Nov 2009 10:51:03 -0500 (EST) X-Virus-Scanned: clamav-milter 0.95.1 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-2.5 required=4.2 tests=AWL,BAYES_00,RDNS_NONE autolearn=no version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on bigwig.baldwin.cx Cc: Warner Losh , freebsd-new-bus@freebsd.org, Scott Long , Ed Maste Subject: Re: [PATCH] Buffer overflow in devclass_add_device() X-BeenThere: freebsd-new-bus@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: FreeBSD's new-bus architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Nov 2009 15:51:04 -0000 On Friday 06 November 2009 10:20:35 am Attilio Rao wrote: > A buffer overflow is possible in devclass_add_device(). > More specifically, the dev nameunit construction is based on the > assumption that the unit linked with the device is invariant but that > can change when calling devclass_alloc_unit() (because -1 is passed > or, more simply, because the unit choosen is beyond the table limits). > This results in a buffer overflow if the bug is too short on the > second snprintf(). > This patch should fix it: > http://www.freebsd.org/~attilio/Sandvine/STABLE_8/subr_bus/subr_bus.diff > > aiming for the max possible number of digits necessary. > This bug has been found by Sandvine Incorporated. > Please reivew. Looks ok to me. -- John Baldwin From owner-freebsd-new-bus@FreeBSD.ORG Fri Nov 6 15:52:08 2009 Return-Path: Delivered-To: freebsd-new-bus@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id A7D921065670 for ; Fri, 6 Nov 2009 15:52:08 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-bw0-f213.google.com (mail-bw0-f213.google.com [209.85.218.213]) by mx1.freebsd.org (Postfix) with ESMTP id 3625E8FC19 for ; Fri, 6 Nov 2009 15:52:07 +0000 (UTC) Received: by bwz5 with SMTP id 5so1328479bwz.3 for ; Fri, 06 Nov 2009 07:52:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:sender:received:date :x-google-sender-auth:message-id:subject:from:to:content-type; bh=GlyZ0FVtfLCaSi157jeps3pgNF4pluqSjH3+xhAsZ1g=; b=Ov3p/qn1SCr9inmuLeUJaFJM5TXfOnfgFpkn+1QwfGXIqdK7EEwwMNuuGPscWtm3nf fnsEvXyZFEyzceVzTbNieZjJrG8mEkWSUZR1lYPjyJ8RBbW4Vas224loX+lfS/Sg0pIX DNdySC2POkjX5v9vObjKG0gIw5ZERUANpsSXk= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:date:x-google-sender-auth:message-id:subject :from:to:content-type; b=hsgFZZfIoftaM/fnx/nLi9o5fAYVaoB5r3kwlAtcyo6rfUq51PQVm3KOZcOxAMQcaD lpTgtcrUIBHHSH7dzQzO13mRCzAxIkpeiRQodFcmqVdeZss9VMuktdHU4KjwhxXctR92 WW2I/NqZK25ev2yMt5QcBXkqiN8z9QF89crFQ= MIME-Version: 1.0 Sender: asmrookie@gmail.com Received: by 10.223.14.140 with SMTP id g12mr649958faa.50.1257520835626; Fri, 06 Nov 2009 07:20:35 -0800 (PST) Date: Fri, 6 Nov 2009 16:20:35 +0100 X-Google-Sender-Auth: 75d25ff71dc1da1d Message-ID: <3bbf2fe10911060720m6d6919ffw91dcc5b6c1c2016a@mail.gmail.com> From: Attilio Rao To: freebsd-new-bus@freebsd.org, John Baldwin , Scott Long , Warner Losh , Ed Maste Content-Type: text/plain; charset=UTF-8 Cc: Subject: [PATCH] Buffer overflow in devclass_add_device() X-BeenThere: freebsd-new-bus@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: FreeBSD's new-bus architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Nov 2009 15:52:08 -0000 A buffer overflow is possible in devclass_add_device(). More specifically, the dev nameunit construction is based on the assumption that the unit linked with the device is invariant but that can change when calling devclass_alloc_unit() (because -1 is passed or, more simply, because the unit choosen is beyond the table limits). This results in a buffer overflow if the bug is too short on the second snprintf(). This patch should fix it: http://www.freebsd.org/~attilio/Sandvine/STABLE_8/subr_bus/subr_bus.diff aiming for the max possible number of digits necessary. This bug has been found by Sandvine Incorporated. Please reivew. Thanks, Attilio -- Peace can only be achieved by understanding - A. Einstein From owner-freebsd-new-bus@FreeBSD.ORG Fri Nov 6 16:21:11 2009 Return-Path: Delivered-To: freebsd-new-bus@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id AAD4E1065695; Fri, 6 Nov 2009 16:21:11 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from harmony.bsdimp.com (bsdimp.com [199.45.160.85]) by mx1.freebsd.org (Postfix) with ESMTP id 689588FC20; Fri, 6 Nov 2009 16:21:11 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by harmony.bsdimp.com (8.14.3/8.14.1) with ESMTP id nA6GFenb085596; Fri, 6 Nov 2009 09:15:40 -0700 (MST) (envelope-from imp@bsdimp.com) Date: Fri, 06 Nov 2009 09:15:43 -0700 (MST) Message-Id: <20091106.091543.2076840904.imp@bsdimp.com> To: attilio@FreeBSD.org From: "M. Warner Losh" In-Reply-To: <3bbf2fe10911060720m6d6919ffw91dcc5b6c1c2016a@mail.gmail.com> References: <3bbf2fe10911060720m6d6919ffw91dcc5b6c1c2016a@mail.gmail.com> X-Mailer: Mew version 5.2 on Emacs 21.3 / Mule 5.0 (SAKAKI) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit Cc: freebsd-new-bus@FreeBSD.org, scottl@FreeBSD.org, emaste@sandvine.com, jhb@FreeBSD.org Subject: Re: [PATCH] Buffer overflow in devclass_add_device() X-BeenThere: freebsd-new-bus@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: FreeBSD's new-bus architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Nov 2009 16:21:11 -0000 In message: <3bbf2fe10911060720m6d6919ffw91dcc5b6c1c2016a@mail.gmail.com> Attilio Rao writes: : A buffer overflow is possible in devclass_add_device(). : More specifically, the dev nameunit construction is based on the : assumption that the unit linked with the device is invariant but that : can change when calling devclass_alloc_unit() (because -1 is passed : or, more simply, because the unit choosen is beyond the table limits). : This results in a buffer overflow if the bug is too short on the : second snprintf(). : This patch should fix it: : http://www.freebsd.org/~attilio/Sandvine/STABLE_8/subr_bus/subr_bus.diff : : aiming for the max possible number of digits necessary. : This bug has been found by Sandvine Incorporated. : Please reivew. I don't see a problem with it, except you'd want -INT_MAX to be paranoid, since it is one character longer (or just add 1) :) However, it might be better to just allocate strlen(dc->name) + log10(INT_MAX) + 2 and not have snprintf do that calculation. But it doesn't look like there's a compile-time constant for that... Warner From owner-freebsd-new-bus@FreeBSD.ORG Fri Nov 6 16:22:38 2009 Return-Path: Delivered-To: freebsd-new-bus@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 7F5A3106566B; Fri, 6 Nov 2009 16:22:38 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-fx0-f227.google.com (mail-fx0-f227.google.com [209.85.220.227]) by mx1.freebsd.org (Postfix) with ESMTP id BBA0B8FC12; Fri, 6 Nov 2009 16:22:37 +0000 (UTC) Received: by fxm27 with SMTP id 27so286580fxm.3 for ; Fri, 06 Nov 2009 08:22:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:sender:received:in-reply-to :references:date:x-google-sender-auth:message-id:subject:from:to:cc :content-type; bh=gs6AJOmdSDwPe8i787BzduMD+KlE9Xi7F3SfxyYHYFw=; b=TcEIV9QjqKCor26smMuMIMRt8emZvrblZN4m7VFp/I6z35aEQHSQhWiGmkte10L+po 1ccrAXeLkBirn+YEQqfHWABgAIG7aLOcLgG41bAdLlEYsE+8RTdQEZE/Ud70ignTzj/k 5YZC15ZsqU9yV7oLObkfHgLzc+tF3YdyKTWok= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; b=d/8ujNUR4Q6+SQGQ5ClBW18OCI4ol1/fdSdoCLvWKfGlup4qsQwxjgX/M8vg/q9t16 l5OPoawv4cCo79OqsT+NJVNjZ1pYmC88RvZ5DHjU3ECUdUwPFLAeVo7e+MyaGRPV/LSJ 5hLDvLEwOY9lnnkflP2L8eB9QUY2FPskH1nS8= MIME-Version: 1.0 Sender: asmrookie@gmail.com Received: by 10.223.4.137 with SMTP id 9mr689643far.95.1257524556646; Fri, 06 Nov 2009 08:22:36 -0800 (PST) In-Reply-To: <20091106.091543.2076840904.imp@bsdimp.com> References: <3bbf2fe10911060720m6d6919ffw91dcc5b6c1c2016a@mail.gmail.com> <20091106.091543.2076840904.imp@bsdimp.com> Date: Fri, 6 Nov 2009 17:22:36 +0100 X-Google-Sender-Auth: 50616e67bd1f94dd Message-ID: <3bbf2fe10911060822g35b81099ib6fa53473d7c20fe@mail.gmail.com> From: Attilio Rao To: "M. Warner Losh" Content-Type: text/plain; charset=UTF-8 Cc: freebsd-new-bus@freebsd.org, scottl@freebsd.org, emaste@sandvine.com Subject: Re: [PATCH] Buffer overflow in devclass_add_device() X-BeenThere: freebsd-new-bus@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: FreeBSD's new-bus architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Nov 2009 16:22:38 -0000 2009/11/6 M. Warner Losh : > In message: <3bbf2fe10911060720m6d6919ffw91dcc5b6c1c2016a@mail.gmail.com> > Attilio Rao writes: > : A buffer overflow is possible in devclass_add_device(). > : More specifically, the dev nameunit construction is based on the > : assumption that the unit linked with the device is invariant but that > : can change when calling devclass_alloc_unit() (because -1 is passed > : or, more simply, because the unit choosen is beyond the table limits). > : This results in a buffer overflow if the bug is too short on the > : second snprintf(). > : This patch should fix it: > : http://www.freebsd.org/~attilio/Sandvine/STABLE_8/subr_bus/subr_bus.diff > : > : aiming for the max possible number of digits necessary. > : This bug has been found by Sandvine Incorporated. > : Please reivew. > > I don't see a problem with it, except you'd want -INT_MAX to be > paranoid, since it is one character longer (or just add 1) :) I don't think that unit number can grow negative, can they? Thanks, Attilio -- Peace can only be achieved by understanding - A. Einstein From owner-freebsd-new-bus@FreeBSD.ORG Fri Nov 6 16:45:30 2009 Return-Path: Delivered-To: freebsd-new-bus@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 7A141106566B; Fri, 6 Nov 2009 16:45:30 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 4A98B8FC15; Fri, 6 Nov 2009 16:45:30 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id EA01B46B2D; Fri, 6 Nov 2009 11:45:29 -0500 (EST) Received: from jhbbsd.hudson-trading.com (unknown [209.249.190.8]) by bigwig.baldwin.cx (Postfix) with ESMTPA id 0FF238A01D; Fri, 6 Nov 2009 11:45:29 -0500 (EST) From: John Baldwin To: "M. Warner Losh" Date: Fri, 6 Nov 2009 11:45:18 -0500 User-Agent: KMail/1.9.7 References: <3bbf2fe10911060720m6d6919ffw91dcc5b6c1c2016a@mail.gmail.com> <20091106.091543.2076840904.imp@bsdimp.com> In-Reply-To: <20091106.091543.2076840904.imp@bsdimp.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200911061145.19212.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.0.1 (bigwig.baldwin.cx); Fri, 06 Nov 2009 11:45:29 -0500 (EST) X-Virus-Scanned: clamav-milter 0.95.1 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-2.5 required=4.2 tests=AWL,BAYES_00,RDNS_NONE autolearn=no version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on bigwig.baldwin.cx Cc: attilio@freebsd.org, freebsd-new-bus@freebsd.org, scottl@freebsd.org, emaste@sandvine.com Subject: Re: [PATCH] Buffer overflow in devclass_add_device() X-BeenThere: freebsd-new-bus@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: FreeBSD's new-bus architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Nov 2009 16:45:30 -0000 On Friday 06 November 2009 11:15:43 am M. Warner Losh wrote: > In message: <3bbf2fe10911060720m6d6919ffw91dcc5b6c1c2016a@mail.gmail.com> > Attilio Rao writes: > : A buffer overflow is possible in devclass_add_device(). > : More specifically, the dev nameunit construction is based on the > : assumption that the unit linked with the device is invariant but that > : can change when calling devclass_alloc_unit() (because -1 is passed > : or, more simply, because the unit choosen is beyond the table limits). > : This results in a buffer overflow if the bug is too short on the > : second snprintf(). > : This patch should fix it: > : http://www.freebsd.org/~attilio/Sandvine/STABLE_8/subr_bus/subr_bus.diff > : > : aiming for the max possible number of digits necessary. > : This bug has been found by Sandvine Incorporated. > : Please reivew. > > I don't see a problem with it, except you'd want -INT_MAX to be > paranoid, since it is one character longer (or just add 1) :) > > However, it might be better to just allocate strlen(dc->name) + > log10(INT_MAX) + 2 and not have snprintf do that calculation. But it > doesn't look like there's a compile-time constant for that... In this case I think the snprintf() is fine as code-wise I think it is simpler (it matches up well with the later snprintf() to fill out the buffer). Given that adding devices isn't generally a critical-path, I think the clarity is worth the probably quite small additional cost of snprintf(). -- John Baldwin From owner-freebsd-new-bus@FreeBSD.ORG Fri Nov 6 16:49:47 2009 Return-Path: Delivered-To: freebsd-new-bus@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id C247F106566B; Fri, 6 Nov 2009 16:49:47 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from harmony.bsdimp.com (bsdimp.com [199.45.160.85]) by mx1.freebsd.org (Postfix) with ESMTP id 7CE8E8FC1B; Fri, 6 Nov 2009 16:49:47 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by harmony.bsdimp.com (8.14.3/8.14.1) with ESMTP id nA6GcibW085903; Fri, 6 Nov 2009 09:38:44 -0700 (MST) (envelope-from imp@bsdimp.com) Date: Fri, 06 Nov 2009 09:38:47 -0700 (MST) Message-Id: <20091106.093847.1347313226.imp@bsdimp.com> To: attilio@freebsd.org From: "M. Warner Losh" In-Reply-To: <3bbf2fe10911060822g35b81099ib6fa53473d7c20fe@mail.gmail.com> References: <3bbf2fe10911060720m6d6919ffw91dcc5b6c1c2016a@mail.gmail.com> <20091106.091543.2076840904.imp@bsdimp.com> <3bbf2fe10911060822g35b81099ib6fa53473d7c20fe@mail.gmail.com> X-Mailer: Mew version 5.2 on Emacs 21.3 / Mule 5.0 (SAKAKI) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit Cc: freebsd-new-bus@freebsd.org, scottl@freebsd.org, emaste@sandvine.com Subject: Re: [PATCH] Buffer overflow in devclass_add_device() X-BeenThere: freebsd-new-bus@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: FreeBSD's new-bus architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Nov 2009 16:49:47 -0000 In message: <3bbf2fe10911060822g35b81099ib6fa53473d7c20fe@mail.gmail.com> Attilio Rao writes: : 2009/11/6 M. Warner Losh : : > In message: <3bbf2fe10911060720m6d6919ffw91dcc5b6c1c2016a@mail.gmail.com> : > Attilio Rao writes: : > : A buffer overflow is possible in devclass_add_device(). : > : More specifically, the dev nameunit construction is based on the : > : assumption that the unit linked with the device is invariant but that : > : can change when calling devclass_alloc_unit() (because -1 is passed : > : or, more simply, because the unit choosen is beyond the table limits). : > : This results in a buffer overflow if the bug is too short on the : > : second snprintf(). : > : This patch should fix it: : > : http://www.freebsd.org/~attilio/Sandvine/STABLE_8/subr_bus/subr_bus.diff : > : : > : aiming for the max possible number of digits necessary. : > : This bug has been found by Sandvine Incorporated. : > : Please reivew. : > : > I don't see a problem with it, except you'd want -INT_MAX to be : > paranoid, since it is one character longer (or just add 1) :) : : I don't think that unit number can grow negative, can they? They can't, but this is about an abundance of caution, right? Warner