Buffer overflow in devclass_add_device()

[Attilio Rao-2]

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]"

[John Baldwin]

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]"

[Warner Losh]

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]"

[Attilio Rao-2]

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]"

[Warner Losh]

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]"

[John Baldwin]

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]"