Re: CVE-2010-1173 kernel: skb_over_panic resulting from multiple invalid parameter errors

[Hui Zhu <hui.zhu () windriver com>]

Eugene Teo:
> https://bugzilla.redhat.com/CVE-2010-1173
> http://article.gmane.org/gmane.linux.network/159531
>
> Reported by Chris Guo from Nokia China via Red Hat Support. A similar
> issue was reported by Jukka Taimisto and Olli Jarva from Codenomicon Ltd
> via CERT-FI. This was also reported by Windriver on behalf of their
> customer via vendor-sec.
>
> Kernel crash occurs if sctp listening port receives malformatted init
> package.
>
> Its an skb_over_panic BUG halt that results from processing an init
> chunk in which too many of its variable length parameters are in some
> way malformed.
>
> The problem is in sctp_process_unk_param:
> if (NULL == *errp)
>  *errp = sctp_make_op_error_space(asoc, chunk,
>       ntohs(chunk->chunk_hdr->length));
>
>  if (*errp) {
>   sctp_init_cause(*errp, SCTP_ERROR_UNKNOWN_PARAM,
>      WORD_ROUND(ntohs(param.p->length)));
>   sctp_addto_chunk(*errp,
>    WORD_ROUND(ntohs(param.p->length)),
>       param.v);
>
> When we allocate an error chunk, we assume that the worst case scenario
> requires that we have chunk_hdr->length data allocated, which would be
> correct nominally, given that we call sctp_addto_chunk for the violating
> parameter. Unfortunately, we also, in sctp_init_cause insert a
> sctp_errhdr_t structure into the error chunk, so the worst case
> situation in which all parameters are in violation requires
> chunk_hdr->length+(sizeof(sctp_errhdr_t)*param_count) bytes of data.
>
> This fix solves the problem by allowing our implementation to only
> report a fixed number of errors.  When we encounter an error in
> parameter processing we allocate a chunk that is min(asoc->pathmtu,
> SCTP_DEFAULT_MAXSEGMENT), limiting our error reporting to a single mtu
> sized chunk.  Parameter errors that grow beyond that value are discarded.
>
> Thanks, Eugene

Forward a mail of my workmate:

Make a larger chunk is acceptable but for common communication case, the original one is fine enough. And I don't think 
it is necessary to make such a big chunk. In fact, in worst case, a *double* sized chunk is large enough to hold all 
necessary data. So, if we have to use the larger chunk than original, we shoudl use min(double of chunk hdr length, 
asoc->pathmtu, SCTP_DEFAULT_MAXSEGMENT).

For other part of that patch, I think:
-) A logical issue related to function "sctp_init_cause_fixed". Even if it finds the space is not enough and return 
"-ENOSPC", "sctp_addto_chunk_fixed" will still be spawned to add error param data in the error report chunk.

-) As I understand, every error report should has two part: head and data. So if the chunk doesn't have enough space to 
hold it, both of them should be discarded. So in WINDRIVER patch that ZhuHui released out, there are only one space 
check for the whole error report sapce: head + data. But with this patch, it may just report the error data without 
head.

-) No use to check the "skb_tailroom" serially, for example in call trace "sctp_init_cause_fixed -> 
sctp_addto_chunk_fixed". Here it just use the original actp_add_chunk is OK.

-) Make function "sctp_init_cause_fixed", "sctp_addto_chunk_fixed" static inline;


Thanks
Xiao