Date: Fri, 06 Nov 2009 09:15:43 -0700 (MST) From: "M. Warner Losh" <imp@bsdimp.com> To: attilio@FreeBSD.org Cc: freebsd-new-bus@FreeBSD.org, scottl@FreeBSD.org, emaste@sandvine.com, jhb@FreeBSD.org Subject: Re: [PATCH] Buffer overflow in devclass_add_device() Message-ID: <20091106.091543.2076840904.imp@bsdimp.com> In-Reply-To: <3bbf2fe10911060720m6d6919ffw91dcc5b6c1c2016a@mail.gmail.com> References: <3bbf2fe10911060720m6d6919ffw91dcc5b6c1c2016a@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
In message: <3bbf2fe10911060720m6d6919ffw91dcc5b6c1c2016a@mail.gmail.com>
Attilio Rao <attilio@FreeBSD.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20091106.091543.2076840904.imp>