Neil Horman wrote:
> On Wed, Apr 28, 2010 at 01:52:05PM -0400, Vlad Yasevich wrote:
>>
>> Vlad Yasevich wrote:
>>> Neil Horman wrote:
>>>> On Wed, Apr 28, 2010 at 10:00:37AM -0400, Vlad Yasevich wrote:
>>>>> I have this patch and a few others already queued.
>>>>>
>>>>> I was planning on sending these today for stable.
>>>>>
>>>>> Here is the full list of stable patches I have:
>>>>>
>>>>> sctp: Fix oops when sending queued ASCONF chunks
>>>>> sctp: fix to calc the INIT/INIT-ACK chunk length correctly is set
>>>>> sctp: per_cpu variables should be in bh_disabled section
>>>>> sctp: fix potential reference of a freed pointer
>>>>> sctp: avoid irq lock inversion while call sk->sk_data_ready()
>>>>>
>>>>> -vlad
>>>>>
>>>> Are you sure? this oops looks _very_ simmilar to the INIT/INIT-ACK length
>>>> calculation oops described above, but is in fact different, and requires this
>>>> patch, from what I can see. The right fix might be in the ASCONF chunk patch
>>>> you list above, but I don't see that in your tree at the moment, so I can't be
>>>> sure.
>>> As I said, I totally goofed when reading the description and I apologize.
>>> However, I do one comment regarding the patch.
>>>
>>> If the bad packet is REALLY long (I mean close to 65K IP limit), then
>>> we'll end up allocating a supper huge skb in this case and potentially exceed
>>> the IP length limitation. Section 11.4 of rfc 4960 allows us to omit some
>>> errors and limit the size of the packet.
>>>
>>> I would recommend limiting this to MTU worth of potentiall errors. This is
>>> on top of what the INIT-ACK is going to carry, so at most we'll sent 2 MTUs
>>> worth. That's still a potential by amplification attack, but it's somewhat
>>> mitigated.
>>>
>>> Of course now we have to handle the case of checking for space before adding
>>> an error cause. :)
>>>
>> Hi Neil
>>
>> I am also not crazy about the pre-allocation scheme. In the case where you have
>> say 100 parameters that are all 'skip' parameters, you'd end up pre-allocating a
>> huge buffer for absolutely nothing.
>>
> Would have been nice if you'd made your opinion known 4 hours ago when I was
> testing version 2 of this. :)
>
sorry, fighting a head cold and need drugs to think clearly... ;)
>> This is another point toward a fixed error chunk size and let parameter
>> processing allocate it when it reaches a parameter that needs an error.
>>
> Hmm, ok, what would you say to a pathmtu sized chunk allocation in parameter
> processing that drops errors beyond its capacity
> Neil
Here is my quick take on this. Haven't tested it at all.
-vlad
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 0fd5b4c..74d8d21 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1959,8 +1959,10 @@ static void sctp_process_ext_param(struct sctp_association *asoc,
static sctp_ierror_t sctp_process_unk_param(const struct sctp_association *asoc,
union sctp_params param,
struct sctp_chunk *chunk,
- struct sctp_chunk **errp)
+ struct sctp_chunk **errp,
+ unsigned int param_cnt)
{
+ unsigned int needed_bytes;
int retval = SCTP_IERROR_NO_ERROR;
switch (param.p->type & SCTP_PARAM_ACTION_MASK) {
@@ -1976,11 +1978,41 @@ static sctp_ierror_t sctp_process_unk_param(const struct sctp_association *asoc,
/* Make an ERROR chunk, preparing enough room for
* returning multiple unknown parameters.
*/
- if (NULL == *errp)
- *errp = sctp_make_op_error_space(asoc, chunk,
- ntohs(chunk->chunk_hdr->length));
+ if (NULL == *errp) {
+ unsigned int len;
+
+ /* Reserver space for the worst possible case
+ * at this time. We count incomming chunk length
+ * since error parameters carry the bad parameter
+ * inself, plus have space for error headers for
+ * the remaining number of parameters.
+ */
+ len = ntohs(chunk->chunk_hdr->length);
+ len += sizeof(sctp_errhdr_t) * param_cnt;
+
+ /* We need to prevent amplification attacks that
+ * result from sending 65K init chunks with all bad
+ * params maliciously, so lets limit our error response
+ * to 1 MTU worth of errors, but at least 1500 bytes
+ * in case our pathmtu hasn't been updated yet.
+ */
+ len = min(len, asoc ? asoc->pathmtu :
+ SCTP_DEFAULT_MAXSEGMENT);
+ *errp = sctp_make_op_error_space(asoc, chunk, len);
+ }
if (*errp) {
+ needed_bytes = sizeof(sctp_errhdr_t) +
+ WORD_ROUND(ntohs(param.p->length));
+
+ if (skb_tailroom((*errp)->skb) < needed_bytes)
+ /*
+ * If we're over our packet size here, don't add
+ * the error, this is allowed by section 11.4 of
+ * RFC 4960
+ */
+ break;
+
sctp_init_cause(*errp, SCTP_ERROR_UNKNOWN_PARAM,
WORD_ROUND(ntohs(param.p->length)));
sctp_addto_chunk(*errp,
@@ -2013,7 +2045,8 @@ static sctp_ierror_t sctp_verify_param(const struct sctp_association *asoc,
union sctp_params param,
sctp_cid_t cid,
struct sctp_chunk *chunk,
- struct sctp_chunk **err_chunk)
+ struct sctp_chunk **err_chunk,
+ unsigned int param_cnt)
{
struct sctp_hmac_algo_param *hmacs;
int retval = SCTP_IERROR_NO_ERROR;
@@ -2119,7 +2152,8 @@ fallthrough:
default:
SCTP_DEBUG_PRINTK("Unrecognized param: %d for chunk %d.\n",
ntohs(param.p->type), cid);
- retval = sctp_process_unk_param(asoc, param, chunk, err_chunk);
+ retval = sctp_process_unk_param(asoc, param, chunk, err_chunk,
+ param_cnt);
break;
}
return retval;
@@ -2135,6 +2169,7 @@ int sctp_verify_init(const struct sctp_association *asoc,
union sctp_params param;
int has_cookie = 0;
int result;
+ unsigned int param_cnt = 0;
/* Verify stream values are non-zero. */
if ((0 == peer_init->init_hdr.num_outbound_streams) ||
@@ -2150,6 +2185,7 @@ int sctp_verify_init(const struct sctp_association *asoc,
if (SCTP_PARAM_STATE_COOKIE == param.p->type)
has_cookie = 1;
+ param_cnt++;
} /* for (loop through all parameters) */
@@ -2173,7 +2209,8 @@ int sctp_verify_init(const struct sctp_association *asoc,
/* Verify all the variable length parameters */
sctp_walk_params(param, peer_init, init_hdr.params) {
- result = sctp_verify_param(asoc, param, cid, chunk, errp);
+ result = sctp_verify_param(asoc, param, cid, chunk, errp,
+ param_cnt);
switch (result) {
case SCTP_IERROR_ABORT:
case SCTP_IERROR_NOMEM:
@@ -2184,6 +2221,7 @@ int sctp_verify_init(const struct sctp_association *asoc,
default:
break;
}
+ param_cnt--;
} /* for (loop through all parameters) */
