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 _______________________________________________ [hidden email] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus To unsubscribe, send any mail to "[hidden email]" |
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 _______________________________________________ [hidden email] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus To unsubscribe, send any mail to "[hidden email]" |
In reply to this post by Attilio Rao-2
In message: <[hidden email]>
Attilio Rao <[hidden email]> 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 _______________________________________________ [hidden email] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus To unsubscribe, send any mail to "[hidden email]" |
2009/11/6 M. Warner Losh <[hidden email]>:
> In message: <[hidden email]> > Attilio Rao <[hidden email]> 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 _______________________________________________ [hidden email] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus To unsubscribe, send any mail to "[hidden email]" |
In message: <[hidden email]>
Attilio Rao <[hidden email]> writes: : 2009/11/6 M. Warner Losh <[hidden email]>: : > In message: <[hidden email]> : > Attilio Rao <[hidden email]> 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 _______________________________________________ [hidden email] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus To unsubscribe, send any mail to "[hidden email]" |
In reply to this post by Warner Losh
On Friday 06 November 2009 11:15:43 am M. Warner Losh wrote:
> In message: <[hidden email]> > Attilio Rao <[hidden email]> 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 _______________________________________________ [hidden email] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus To unsubscribe, send any mail to "[hidden email]" |
Free forum by Nabble | Edit this page |