Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 6 Nov 2009 11:45:18 -0500
From:      John Baldwin <jhb@freebsd.org>
To:        "M. Warner Losh" <imp@bsdimp.com>
Cc:        attilio@freebsd.org, freebsd-new-bus@freebsd.org, scottl@freebsd.org, emaste@sandvine.com
Subject:   Re: [PATCH] Buffer overflow in devclass_add_device()
Message-ID:  <200911061145.19212.jhb@freebsd.org>
In-Reply-To: <20091106.091543.2076840904.imp@bsdimp.com>
References:  <3bbf2fe10911060720m6d6919ffw91dcc5b6c1c2016a@mail.gmail.com> <20091106.091543.2076840904.imp@bsdimp.com>

next in thread | previous in thread | raw e-mail | index | archive | help
On Friday 06 November 2009 11:15:43 am M. Warner Losh wrote:
> 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...

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



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200911061145.19212.jhb>