Re: possible array out of bounds access in sys/netinet/sctp_output.c

On Sun, Nov 27, 2011 at 5:24 PM, Jilles Tjoelker <jilles_at_stack.nl> wrote:
> On Sun, Nov 27, 2011 at 03:45:36PM +0000, Alexander Best wrote:
>> i've been playing with clang tot and noticed the following error:
>
>> /usr/local/bin/clang -c -O3 -pipe -fno-inline-functions -fno-strict-aliasing -march=core2 -std=c99 -g -fdiagnostics-show-option -fformat-extensions -Wall  -Wcast-qual -Winline -Wmissing-include-dirs  -Wmissing-prototypes -Wnested-externs -Wpointer-arith  -Wredundant-decls -Wstrict-prototypes -Wundef  -Wno-pointer-sign -nostdinc  -I. -I/usr/git-freebsd-head/sys -I/usr/git-freebsd-head/sys/contrib/altq -D_KERNEL -DHAVE_KERNEL_OPTION_HEADERS -include opt_global.h  -fno-omit-frame-pointer -mno-aes -mno-avx -mcmodel=kernel -mno-red-zone -mno-mmx -msoft-float  -fno-asynchronous-unwind-tables -ffreestanding -Wno-error=tautological-compare -Wno-error=shift-count-negative  -Wno-error=shift-count-overflow -Wno-error=shift-overflow -Wno-error=conversion  -Wno-error=empty-body -Wno-error=gnu-designator -Wno-error=format  -Wno-error=format-invalid-specifier -Wno-error=format-extra-args -Werror  /usr/git-freebsd-head/sys/netinet/sctp_output.c
>> clang: warning: argument unused during compilation: '-fformat-extensions'
>> /usr/git-freebsd-head/sys/netinet/sctp_output.c:4685:2: error: array index 1 is past the end of the array (which contains 1 element) [-Werror,-Warray-bounds]
>>         sup_addr->addr_type[1] = htons(SCTP_IPV6_ADDRESS);
>>         ^                   ~
>> /usr/git-freebsd-head/sys/netinet/sctp_header.h:84:2: note: array 'addr_type' declared here
>>         uint16_t addr_type[SCTP_ARRAY_MIN_LEN]; /* array of supported address
>>         ^
>> 1 error generated.
>> *** Error code 1
>>
>> Stop in /usr/obj/usr/git-freebsd-head/sys/GENERIC.
>> *** Error code 1
>>
>> Stop in /usr/git-freebsd-head.
>> *** Error code 1
>>
>> Stop in /usr/git-freebsd-head.
>
>> this is from a GENERIC kernel build (so INET + INET6) for amd64. is this a
>> false positive, or is length(sup_addr->addr_type) really == 1, thus making
>> sup_addr->addr_type[1] an illegal access?
>
> This is the fairly common construct of a variable-length array at the
> end of a struct. With C89, this was not allowed but defining one element
> and allocating more elements worked in most implementations. C99
> recognized this need and created a way to do it, which looks like
> uint16_t addr_type[];. This adds any necessary padding and allows access
> to however many elements have been allocated. Also, if it is not at the
> end of a struct it is an error.
>
> Using this new construct requires code changes because some code such as
> fairly close to the error message relies on the size of the one element
> already in the struct.
>
> --
> Jilles Tjoelker
> _______________________________________________
> freebsd-current_at_freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "freebsd-current-unsubscribe_at_freebsd.org"
>


I looked at sctp_send_initiate() and it seems that independently from
the number of types supported (IPV6/IPV4 or both) two elements are
allocated in the array sup_addr->addr_type[0] . In case only a type is
supported one of the elements is simply padding.
(http://fxr.watson.org/fxr/source/netinet/sctp_output.c#L4670) .

So, this should fix the issue, but maybe I'm wrong so feel free to correct me.

http://davit.altervista.org/sctp_header_types.diff

I defined a new macro mainly because SCTP_ARRAY_MIN_LEN is used in
another place, i.e. in the field name of struct sctp_host_name_param,
defined in sctp_header.h). Thanks to arundel_at_ for testing.

Regards

Davide