oss-sec mailing list archives

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


From: Eugene Teo <eugene () redhat com>
Date: Thu, 29 Apr 2010 11:20:25 +0800

On 04/29/2010 10:58 AM, Hui Zhu wrote:
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;

Hi Hui,

As I already mentioned, discussion of the patch should take place in netdev. You might want to consider CC'ing security () kernel org too. For a start, this is not the right list for such a discussion. I don't even see the maintainer for SCTP cc'ed to the email too. To make it easier for you, feel free to follow-up from here: http://article.gmane.org/gmane.linux.network/159531.

Thanks, Eugene


Current thread: