* Re: [PATCH] sctp: Fix kernel panic while process protocol violation
2008-09-08 1:15 [PATCH] sctp: Fix kernel panic while process protocol violation parameter Wei Yongjun
@ 2008-09-08 15:03 ` Vlad Yasevich
2008-09-09 0:48 ` Wei Yongjun
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Vlad Yasevich @ 2008-09-08 15:03 UTC (permalink / raw)
To: linux-sctp
Wei Yongjun wrote:
> Since call to function sctp_sf_abort_violation() need paramter 'arg' with
> 'struct sctp_chunk' type, it will read the chunk type and chunk length from
> the chunk_hdr member of chunk. But call to sctp_sf_violation_paramlen()
> always with 'struct sctp_paramhdr' type's parameter, it will be passed to
> sctp_sf_abort_violation(). This may cause kernel panic.
>
> sctp_sf_violation_paramlen()
> |-- sctp_sf_abort_violation()
> |-- sctp_make_abort_violation()
>
> This patch fixed this problem by add a new paramter 'struct sctp_paramhdr'
> to sctp_make_abort_violation(), if param is not NULL, encode phdr with
> param,
> if param is NULL, encode phdr with chunk.
>
> This patch also fix two place which called sctp_sf_violation_paramlen()
> with
> wrong paramter type.
GAK!!! Thanks for finding this.
I am not sure I am very happy this approach though...
sctp_sf_violation_paramlen() is only used in processing of ascof/asconf_ack,
so changing generic ABORT generation to track another argument is not the
cleanest solution...
In addition, we also have sctp_process_inv_paramlength() which is almost
the same thing as sctp_sf_violation_paramlen(). So, I think it would
be a good idea to have this code cleaned up and merged into a single
function that can be called from both palaces.
Part of the problem is that INIT processing expects an error chunk instead
of the abort. However, it's being rather dense in that regard and should
be taught how to handle both.
Once that happens, sctp_process_inv_paramlength() can start returning
an ABORT chunk back just like sctp_sf_violation_paramlen(). An once
that happens, we can call this one function from everywhere.
-vlad
>
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> ---
> include/net/sctp/sm.h | 1 +
> net/sctp/sm_make_chunk.c | 11 +++++++++--
> net/sctp/sm_statefuns.c | 29 +++++++++++++++--------------
> 3 files changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
> index 2481173..c080f19 100644
> --- a/include/net/sctp/sm.h
> +++ b/include/net/sctp/sm.h
> @@ -225,6 +225,7 @@ struct sctp_chunk *sctp_make_abort_user(const struct
> sctp_association *,
> const struct msghdr *, size_t msg_len);
> struct sctp_chunk *sctp_make_abort_violation(const struct
> sctp_association *,
> const struct sctp_chunk *,
> + const struct sctp_paramhdr *,
> const __u8 *,
> const size_t );
> struct sctp_chunk *sctp_make_heartbeat(const struct sctp_association *,
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index e8ca4e5..6702901 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -989,6 +989,7 @@ static void *sctp_addto_param(struct sctp_chunk
> *chunk, int len,
> struct sctp_chunk *sctp_make_abort_violation(
> const struct sctp_association *asoc,
> const struct sctp_chunk *chunk,
> + const struct sctp_paramhdr *param,
> const __u8 *payload,
> const size_t paylen)
> {
> @@ -1003,8 +1004,14 @@ struct sctp_chunk *sctp_make_abort_violation(
> sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION, paylen
> + sizeof(sctp_paramhdr_t));
>
> - phdr.type = htons(chunk->chunk_hdr->type);
> - phdr.length = chunk->chunk_hdr->length;
> + if (param != NULL) {
> + phdr.type = param->type;
> + phdr.length = param->length;
> + } else {
> + phdr.type = htons(chunk->chunk_hdr->type);
> + phdr.length = chunk->chunk_hdr->length;
> + }
> +
> sctp_addto_chunk(retval, paylen, payload);
> sctp_addto_param(retval, sizeof(sctp_paramhdr_t), &phdr);
>
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index 8848d32..2b13729 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -103,7 +103,7 @@ static sctp_disposition_t
> sctp_stop_t1_and_abort(sctp_cmd_seq_t *commands,
> static sctp_disposition_t sctp_sf_abort_violation(
> const struct sctp_endpoint *ep,
> const struct sctp_association *asoc,
> - void *arg,
> + void *arg, void *ext,
> sctp_cmd_seq_t *commands,
> const __u8 *payload,
> const size_t paylen);
> @@ -119,7 +119,7 @@ static sctp_disposition_t sctp_sf_violation_paramlen(
> const struct sctp_endpoint *ep,
> const struct sctp_association *asoc,
> const sctp_subtype_t type,
> - void *arg,
> + void *arg, void *ext,
> sctp_cmd_seq_t *commands);
>
> static sctp_disposition_t sctp_sf_violation_ctsn(
> @@ -3425,7 +3425,7 @@ sctp_disposition_t sctp_sf_do_asconf(const struct
> sctp_endpoint *ep,
> addr_param = (union sctp_addr_param *)hdr->params;
> length = ntohs(addr_param->p.length);
> if (length < sizeof(sctp_paramhdr_t))
> - return sctp_sf_violation_paramlen(ep, asoc, type,
> + return sctp_sf_violation_paramlen(ep, asoc, type, arg,
> (void *)addr_param, commands);
>
> /* Verify the ASCONF chunk before processing it. */
> @@ -3433,8 +3433,8 @@ sctp_disposition_t sctp_sf_do_asconf(const struct
> sctp_endpoint *ep,
> (sctp_paramhdr_t *)((void *)addr_param + length),
> (void *)chunk->chunk_end,
> &err_param))
> - return sctp_sf_violation_paramlen(ep, asoc, type,
> - (void *)&err_param, commands);
> + return sctp_sf_violation_paramlen(ep, asoc, type, arg,
> + (void *)err_param, commands);
>
> /* ADDIP 5.2 E1) Compare the value of the serial number to the value
> * the endpoint stored in a new association variable
> @@ -3542,8 +3542,8 @@ sctp_disposition_t sctp_sf_do_asconf_ack(const
> struct sctp_endpoint *ep,
> (sctp_paramhdr_t *)addip_hdr->params,
> (void *)asconf_ack->chunk_end,
> &err_param))
> - return sctp_sf_violation_paramlen(ep, asoc, type,
> - (void *)&err_param, commands);
> + return sctp_sf_violation_paramlen(ep, asoc, type, arg,
> + (void *)err_param, commands);
>
> if (last_asconf) {
> addip_hdr = (sctp_addiphdr_t *)last_asconf->subh.addip_hdr;
> @@ -4103,13 +4103,14 @@ sctp_disposition_t sctp_sf_violation(const
> struct sctp_endpoint *ep,
> static sctp_disposition_t sctp_sf_abort_violation(
> const struct sctp_endpoint *ep,
> const struct sctp_association *asoc,
> - void *arg,
> + void *arg, void *ext,
> sctp_cmd_seq_t *commands,
> const __u8 *payload,
> const size_t paylen)
> {
> struct sctp_packet *packet = NULL;
> struct sctp_chunk *chunk = arg;
> + struct sctp_paramhdr *param = ext;
> struct sctp_chunk *abort = NULL;
>
> /* SCTP-AUTH, Section 6.3:
> @@ -4127,7 +4128,7 @@ static sctp_disposition_t sctp_sf_abort_violation(
> goto discard;
>
> /* Make the abort chunk. */
> - abort = sctp_make_abort_violation(asoc, chunk, payload, paylen);
> + abort = sctp_make_abort_violation(asoc, chunk, param, payload,
> paylen);
> if (!abort)
> goto nomem;
>
> @@ -4227,7 +4228,7 @@ static sctp_disposition_t sctp_sf_violation_chunklen(
> {
> static const char err_str[]="The following chunk had invalid length:";
>
> - return sctp_sf_abort_violation(ep, asoc, arg, commands, err_str,
> + return sctp_sf_abort_violation(ep, asoc, arg, NULL, commands, err_str,
> sizeof(err_str));
> }
>
> @@ -4240,11 +4241,11 @@ static sctp_disposition_t
> sctp_sf_violation_paramlen(
> const struct sctp_endpoint *ep,
> const struct sctp_association *asoc,
> const sctp_subtype_t type,
> - void *arg,
> + void *arg, void *ext,
> sctp_cmd_seq_t *commands) {
> static const char err_str[] = "The following parameter had invalid
> length:";
>
> - return sctp_sf_abort_violation(ep, asoc, arg, commands, err_str,
> + return sctp_sf_abort_violation(ep, asoc, arg, ext, commands, err_str,
> sizeof(err_str));
> }
>
> @@ -4263,7 +4264,7 @@ static sctp_disposition_t sctp_sf_violation_ctsn(
> {
> static const char err_str[]="The cumulative tsn ack beyond the max
> tsn currently sent:";
>
> - return sctp_sf_abort_violation(ep, asoc, arg, commands, err_str,
> + return sctp_sf_abort_violation(ep, asoc, arg, NULL, commands, err_str,
> sizeof(err_str));
> }
>
> @@ -4285,7 +4286,7 @@ static sctp_disposition_t sctp_sf_violation_chunk(
> if (!asoc)
> return sctp_sf_violation(ep, asoc, type, arg, commands);
>
> - return sctp_sf_abort_violation(ep, asoc, arg, commands, err_str,
> + return sctp_sf_abort_violation(ep, asoc, arg, NULL, commands, err_str,
> sizeof(err_str));
> }
> /***************************************************************************
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] sctp: Fix kernel panic while process protocol violation
2008-09-08 1:15 [PATCH] sctp: Fix kernel panic while process protocol violation parameter Wei Yongjun
2008-09-08 15:03 ` [PATCH] sctp: Fix kernel panic while process protocol violation Vlad Yasevich
@ 2008-09-09 0:48 ` Wei Yongjun
2008-09-19 9:34 ` Wei Yongjun
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Wei Yongjun @ 2008-09-09 0:48 UTC (permalink / raw)
To: linux-sctp
Vlad Yasevich wrote:
> Wei Yongjun wrote:
>
>> Since call to function sctp_sf_abort_violation() need paramter 'arg' with
>> 'struct sctp_chunk' type, it will read the chunk type and chunk length from
>> the chunk_hdr member of chunk. But call to sctp_sf_violation_paramlen()
>> always with 'struct sctp_paramhdr' type's parameter, it will be passed to
>> sctp_sf_abort_violation(). This may cause kernel panic.
>>
>> sctp_sf_violation_paramlen()
>> |-- sctp_sf_abort_violation()
>> |-- sctp_make_abort_violation()
>>
>> This patch fixed this problem by add a new paramter 'struct sctp_paramhdr'
>> to sctp_make_abort_violation(), if param is not NULL, encode phdr with
>> param,
>> if param is NULL, encode phdr with chunk.
>>
>> This patch also fix two place which called sctp_sf_violation_paramlen()
>> with
>> wrong paramter type.
>>
>
> GAK!!! Thanks for finding this.
>
> I am not sure I am very happy this approach though...
>
> sctp_sf_violation_paramlen() is only used in processing of ascof/asconf_ack,
> so changing generic ABORT generation to track another argument is not the
> cleanest solution...
>
> In addition, we also have sctp_process_inv_paramlength() which is almost
> the same thing as sctp_sf_violation_paramlen(). So, I think it would
> be a good idea to have this code cleaned up and merged into a single
> function that can be called from both palaces.
>
> Part of the problem is that INIT processing expects an error chunk instead
> of the abort. However, it's being rather dense in that regard and should
> be taught how to handle both.
>
> Once that happens, sctp_process_inv_paramlength() can start returning
> an ABORT chunk back just like sctp_sf_violation_paramlen(). An once
> that happens, we can call this one function from everywhere.
>
I'll have a try, thanks.
Wei Yongjun
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] sctp: Fix kernel panic while process protocol violation
2008-09-08 1:15 [PATCH] sctp: Fix kernel panic while process protocol violation parameter Wei Yongjun
2008-09-08 15:03 ` [PATCH] sctp: Fix kernel panic while process protocol violation Vlad Yasevich
2008-09-09 0:48 ` Wei Yongjun
@ 2008-09-19 9:34 ` Wei Yongjun
2008-09-19 20:14 ` Vlad Yasevich
2008-09-25 21:14 ` [PATCH] sctp: Fix kernel panic while process protocol violation parameter Vlad Yasevich
4 siblings, 0 replies; 7+ messages in thread
From: Wei Yongjun @ 2008-09-19 9:34 UTC (permalink / raw)
To: linux-sctp
Vlad Yasevich wrote:
> Wei Yongjun wrote:
>
>> Since call to function sctp_sf_abort_violation() need paramter 'arg' with
>> 'struct sctp_chunk' type, it will read the chunk type and chunk length from
>> the chunk_hdr member of chunk. But call to sctp_sf_violation_paramlen()
>> always with 'struct sctp_paramhdr' type's parameter, it will be passed to
>> sctp_sf_abort_violation(). This may cause kernel panic.
>>
>> sctp_sf_violation_paramlen()
>> |-- sctp_sf_abort_violation()
>> |-- sctp_make_abort_violation()
>>
>> This patch fixed this problem by add a new paramter 'struct sctp_paramhdr'
>> to sctp_make_abort_violation(), if param is not NULL, encode phdr with
>> param,
>> if param is NULL, encode phdr with chunk.
>>
>> This patch also fix two place which called sctp_sf_violation_paramlen()
>> with
>> wrong paramter type.
>>
>
> GAK!!! Thanks for finding this.
>
> I am not sure I am very happy this approach though...
>
> sctp_sf_violation_paramlen() is only used in processing of ascof/asconf_ack,
> so changing generic ABORT generation to track another argument is not the
> cleanest solution...
>
> In addition, we also have sctp_process_inv_paramlength() which is almost
> the same thing as sctp_sf_violation_paramlen(). So, I think it would
> be a good idea to have this code cleaned up and merged into a single
> function that can be called from both palaces.
>
> Part of the problem is that INIT processing expects an error chunk instead
> of the abort. However, it's being rather dense in that regard and should
> be taught how to handle both.
>
Here error chunk is not need, because this is a fatal error and will
send ABORT which only use
the payload of this chunk. So, used abort has the same effect.
> Once that happens, sctp_process_inv_paramlength() can start returning
> an ABORT chunk back just like sctp_sf_violation_paramlen(). An once
> that happens, we can call this one function from everywhere.
>
[PATCHv2] sctp: Fix kernel panic while process protocol violation parameter
Since call to function sctp_sf_abort_violation() need paramter 'arg' with
'struct sctp_chunk' type, it will read the chunk type and chunk length from
the chunk_hdr member of chunk. But call to sctp_sf_violation_paramlen()
always with 'struct sctp_paramhdr' type's parameter, it will be passed to
sctp_sf_abort_violation(). This may cause kernel panic.
sctp_sf_violation_paramlen()
|-- sctp_sf_abort_violation()
|-- sctp_make_abort_violation()
This patch fixed this problem. This patch also fix two place which called
sctp_sf_violation_paramlen() with wrong paramter type.
Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
include/net/sctp/sm.h | 3 ++
net/sctp/sm_make_chunk.c | 37 +++++++++++++++++++++++------------
net/sctp/sm_statefuns.c | 48 +++++++++++++++++++++++++++++++++++----------
3 files changed, 64 insertions(+), 24 deletions(-)
diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
index 2481173..029a54a 100644
--- a/include/net/sctp/sm.h
+++ b/include/net/sctp/sm.h
@@ -227,6 +227,9 @@ struct sctp_chunk *sctp_make_abort_violation(const struct sctp_association *,
const struct sctp_chunk *,
const __u8 *,
const size_t );
+struct sctp_chunk *sctp_make_violation_paramlen(const struct sctp_association *,
+ const struct sctp_chunk *,
+ struct sctp_paramhdr *);
struct sctp_chunk *sctp_make_heartbeat(const struct sctp_association *,
const struct sctp_transport *,
const void *payload,
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index e8ca4e5..dbc4394 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1012,6 +1012,29 @@ end:
return retval;
}
+struct sctp_chunk *sctp_make_violation_paramlen(
+ const struct sctp_association *asoc,
+ const struct sctp_chunk *chunk,
+ struct sctp_paramhdr *param)
+{
+ struct sctp_chunk *retval;
+ static const char error[] = "The following parameter had invalid length:";
+ size_t payload_len = WORD_ROUND(sizeof(error)) + sizeof(sctp_errhdr_t) +
+ sizeof(sctp_paramhdr_t);
+
+ retval = sctp_make_abort(asoc, chunk, payload_len);
+ if (!retval)
+ goto nodata;
+
+ sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION,
+ sizeof(error) + sizeof(sctp_paramhdr_t));
+ sctp_addto_chunk(retval, sizeof(error), error);
+ sctp_addto_param(retval, sizeof(sctp_paramhdr_t), param);
+
+nodata:
+ return retval;
+}
+
/* Make a HEARTBEAT chunk. */
struct sctp_chunk *sctp_make_heartbeat(const struct sctp_association *asoc,
const struct sctp_transport *transport,
@@ -1782,11 +1805,6 @@ static int sctp_process_inv_paramlength(const struct sctp_association *asoc,
const struct sctp_chunk *chunk,
struct sctp_chunk **errp)
{
- static const char error[] = "The following parameter had invalid length:";
- size_t payload_len = WORD_ROUND(sizeof(error)) +
- sizeof(sctp_paramhdr_t);
-
-
/* This is a fatal error. Any accumulated non-fatal errors are
* not reported.
*/
@@ -1794,14 +1812,7 @@ static int sctp_process_inv_paramlength(const struct sctp_association *asoc,
sctp_chunk_free(*errp);
/* Create an error chunk and fill it in with our payload. */
- *errp = sctp_make_op_error_space(asoc, chunk, payload_len);
-
- if (*errp) {
- sctp_init_cause(*errp, SCTP_ERROR_PROTO_VIOLATION,
- sizeof(error) + sizeof(sctp_paramhdr_t));
- sctp_addto_chunk(*errp, sizeof(error), error);
- sctp_addto_param(*errp, sizeof(sctp_paramhdr_t), param);
- }
+ *errp = sctp_make_violation_paramlen(asoc, chunk, param);
return 0;
}
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 8848d32..7c622af 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -119,7 +119,7 @@ static sctp_disposition_t sctp_sf_violation_paramlen(
const struct sctp_endpoint *ep,
const struct sctp_association *asoc,
const sctp_subtype_t type,
- void *arg,
+ void *arg, void *ext,
sctp_cmd_seq_t *commands);
static sctp_disposition_t sctp_sf_violation_ctsn(
@@ -3425,7 +3425,7 @@ sctp_disposition_t sctp_sf_do_asconf(const struct sctp_endpoint *ep,
addr_param = (union sctp_addr_param *)hdr->params;
length = ntohs(addr_param->p.length);
if (length < sizeof(sctp_paramhdr_t))
- return sctp_sf_violation_paramlen(ep, asoc, type,
+ return sctp_sf_violation_paramlen(ep, asoc, type, arg,
(void *)addr_param, commands);
/* Verify the ASCONF chunk before processing it. */
@@ -3433,8 +3433,8 @@ sctp_disposition_t sctp_sf_do_asconf(const struct sctp_endpoint *ep,
(sctp_paramhdr_t *)((void *)addr_param + length),
(void *)chunk->chunk_end,
&err_param))
- return sctp_sf_violation_paramlen(ep, asoc, type,
- (void *)&err_param, commands);
+ return sctp_sf_violation_paramlen(ep, asoc, type, arg,
+ (void *)err_param, commands);
/* ADDIP 5.2 E1) Compare the value of the serial number to the value
* the endpoint stored in a new association variable
@@ -3542,8 +3542,8 @@ sctp_disposition_t sctp_sf_do_asconf_ack(const struct sctp_endpoint *ep,
(sctp_paramhdr_t *)addip_hdr->params,
(void *)asconf_ack->chunk_end,
&err_param))
- return sctp_sf_violation_paramlen(ep, asoc, type,
- (void *)&err_param, commands);
+ return sctp_sf_violation_paramlen(ep, asoc, type, arg,
+ (void *)err_param, commands);
if (last_asconf) {
addip_hdr = (sctp_addiphdr_t *)last_asconf->subh.addip_hdr;
@@ -4240,12 +4240,38 @@ static sctp_disposition_t sctp_sf_violation_paramlen(
const struct sctp_endpoint *ep,
const struct sctp_association *asoc,
const sctp_subtype_t type,
- void *arg,
- sctp_cmd_seq_t *commands) {
- static const char err_str[] = "The following parameter had invalid length:";
+ void *arg, void *ext,
+ sctp_cmd_seq_t *commands)
+{
+ struct sctp_chunk *chunk = arg;
+ struct sctp_paramhdr *param = ext;
+ struct sctp_chunk *abort = NULL;
- return sctp_sf_abort_violation(ep, asoc, arg, commands, err_str,
- sizeof(err_str));
+ if (sctp_auth_recv_cid(SCTP_CID_ABORT, asoc))
+ goto discard;
+
+ /* Make the abort chunk. */
+ abort = sctp_make_violation_paramlen(asoc, chunk, param);
+ if (!abort)
+ goto nomem;
+
+ sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
+ SCTP_INC_STATS(SCTP_MIB_OUTCTRLCHUNKS);
+
+ sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
+ SCTP_ERROR(ECONNABORTED));
+ sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
+ SCTP_PERR(SCTP_ERROR_PROTO_VIOLATION));
+ SCTP_DEC_STATS(SCTP_MIB_CURRESTAB);
+
+discard:
+ sctp_sf_pdiscard(ep, asoc, SCTP_ST_CHUNK(0), arg, commands);
+
+ SCTP_INC_STATS(SCTP_MIB_ABORTEDS);
+
+ return SCTP_DISPOSITION_ABORT;
+nomem:
+ return SCTP_DISPOSITION_NOMEM;
}
/* Handle a protocol violation when the peer trying to advance the
--
1.5.3.8
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] sctp: Fix kernel panic while process protocol violation
2008-09-08 1:15 [PATCH] sctp: Fix kernel panic while process protocol violation parameter Wei Yongjun
` (2 preceding siblings ...)
2008-09-19 9:34 ` Wei Yongjun
@ 2008-09-19 20:14 ` Vlad Yasevich
2008-09-25 21:14 ` [PATCH] sctp: Fix kernel panic while process protocol violation parameter Vlad Yasevich
4 siblings, 0 replies; 7+ messages in thread
From: Vlad Yasevich @ 2008-09-19 20:14 UTC (permalink / raw)
To: linux-sctp
Wei Yongjun wrote:
> Vlad Yasevich wrote:
>> Wei Yongjun wrote:
>>
>>> Since call to function sctp_sf_abort_violation() need paramter 'arg'
>>> with
>>> 'struct sctp_chunk' type, it will read the chunk type and chunk
>>> length from
>>> the chunk_hdr member of chunk. But call to sctp_sf_violation_paramlen()
>>> always with 'struct sctp_paramhdr' type's parameter, it will be
>>> passed to
>>> sctp_sf_abort_violation(). This may cause kernel panic.
>>>
>>> sctp_sf_violation_paramlen()
>>> |-- sctp_sf_abort_violation()
>>> |-- sctp_make_abort_violation()
>>>
>>> This patch fixed this problem by add a new paramter 'struct
>>> sctp_paramhdr'
>>> to sctp_make_abort_violation(), if param is not NULL, encode phdr with
>>> param,
>>> if param is NULL, encode phdr with chunk.
>>>
>>> This patch also fix two place which called sctp_sf_violation_paramlen()
>>> with
>>> wrong paramter type.
>>>
>>
>> GAK!!! Thanks for finding this.
>>
>> I am not sure I am very happy this approach though...
>>
>> sctp_sf_violation_paramlen() is only used in processing of
>> ascof/asconf_ack,
>> so changing generic ABORT generation to track another argument is not the
>> cleanest solution...
>>
>> In addition, we also have sctp_process_inv_paramlength() which is almost
>> the same thing as sctp_sf_violation_paramlen(). So, I think it would
>> be a good idea to have this code cleaned up and merged into a single
>> function that can be called from both palaces.
>>
>> Part of the problem is that INIT processing expects an error chunk
>> instead
>> of the abort. However, it's being rather dense in that regard and should
>> be taught how to handle both.
>>
>
> Here error chunk is not need, because this is a fatal error and will
> send ABORT which only use
> the payload of this chunk. So, used abort has the same effect.
>> Once that happens, sctp_process_inv_paramlength() can start returning
>> an ABORT chunk back just like sctp_sf_violation_paramlen(). An once
>> that happens, we can call this one function from everywhere.
>>
> [PATCHv2] sctp: Fix kernel panic while process protocol violation parameter
>
> Since call to function sctp_sf_abort_violation() need paramter 'arg' with
> 'struct sctp_chunk' type, it will read the chunk type and chunk length from
> the chunk_hdr member of chunk. But call to sctp_sf_violation_paramlen()
> always with 'struct sctp_paramhdr' type's parameter, it will be passed to
> sctp_sf_abort_violation(). This may cause kernel panic.
>
> sctp_sf_violation_paramlen()
> |-- sctp_sf_abort_violation()
> |-- sctp_make_abort_violation()
>
> This patch fixed this problem. This patch also fix two place which called
> sctp_sf_violation_paramlen() with wrong paramter type.
>
Ok. I like this much better. For the bug fix, this is good enough, but
as a next step, we consolidate this a little bit more.
Just one question.
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> ---
> include/net/sctp/sm.h | 3 ++
> net/sctp/sm_make_chunk.c | 37 +++++++++++++++++++++++------------
> net/sctp/sm_statefuns.c | 48
> +++++++++++++++++++++++++++++++++++----------
> 3 files changed, 64 insertions(+), 24 deletions(-)
>
> diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
> index 2481173..029a54a 100644
> --- a/include/net/sctp/sm.h
> +++ b/include/net/sctp/sm.h
> @@ -227,6 +227,9 @@ struct sctp_chunk *sctp_make_abort_violation(const
> struct sctp_association *,
> const struct sctp_chunk *,
> const __u8 *,
> const size_t );
> +struct sctp_chunk *sctp_make_violation_paramlen(const struct
> sctp_association *,
> + const struct sctp_chunk *,
> + struct sctp_paramhdr *);
> struct sctp_chunk *sctp_make_heartbeat(const struct sctp_association *,
> const struct sctp_transport *,
> const void *payload,
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index e8ca4e5..dbc4394 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -1012,6 +1012,29 @@ end:
> return retval;
> }
>
> +struct sctp_chunk *sctp_make_violation_paramlen(
> + const struct sctp_association *asoc,
> + const struct sctp_chunk *chunk,
> + struct sctp_paramhdr *param)
> +{
> + struct sctp_chunk *retval;
> + static const char error[] = "The following parameter had invalid
> length:";
> + size_t payload_len = WORD_ROUND(sizeof(error)) +
> sizeof(sctp_errhdr_t) +
> + sizeof(sctp_paramhdr_t);
Do we really need the WORD_ROUND above. Before we were doing error
chunks that could have contained more then one parameter. But in this
case, we are only going to have this single parameter and the
chunk allocation routing will WORD_ROUND the entire length properly.
-vlad
> +
> + retval = sctp_make_abort(asoc, chunk, payload_len);
> + if (!retval)
> + goto nodata;
> +
> + sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION,
> + sizeof(error) + sizeof(sctp_paramhdr_t));
> + sctp_addto_chunk(retval, sizeof(error), error);
> + sctp_addto_param(retval, sizeof(sctp_paramhdr_t), param);
> +
> +nodata:
> + return retval;
> +}
> +
> /* Make a HEARTBEAT chunk. */
> struct sctp_chunk *sctp_make_heartbeat(const struct sctp_association *asoc,
> const struct sctp_transport *transport,
> @@ -1782,11 +1805,6 @@ static int sctp_process_inv_paramlength(const
> struct sctp_association *asoc,
> const struct sctp_chunk *chunk,
> struct sctp_chunk **errp)
> {
> - static const char error[] = "The following parameter had invalid
> length:";
> - size_t payload_len = WORD_ROUND(sizeof(error)) +
> - sizeof(sctp_paramhdr_t);
> -
> -
> /* This is a fatal error. Any accumulated non-fatal errors are
> * not reported.
> */
> @@ -1794,14 +1812,7 @@ static int sctp_process_inv_paramlength(const
> struct sctp_association *asoc,
> sctp_chunk_free(*errp);
>
> /* Create an error chunk and fill it in with our payload. */
> - *errp = sctp_make_op_error_space(asoc, chunk, payload_len);
> -
> - if (*errp) {
> - sctp_init_cause(*errp, SCTP_ERROR_PROTO_VIOLATION,
> - sizeof(error) + sizeof(sctp_paramhdr_t));
> - sctp_addto_chunk(*errp, sizeof(error), error);
> - sctp_addto_param(*errp, sizeof(sctp_paramhdr_t), param);
> - }
> + *errp = sctp_make_violation_paramlen(asoc, chunk, param);
>
> return 0;
> }
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index 8848d32..7c622af 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -119,7 +119,7 @@ static sctp_disposition_t sctp_sf_violation_paramlen(
> const struct sctp_endpoint *ep,
> const struct sctp_association *asoc,
> const sctp_subtype_t type,
> - void *arg,
> + void *arg, void *ext,
> sctp_cmd_seq_t *commands);
>
> static sctp_disposition_t sctp_sf_violation_ctsn(
> @@ -3425,7 +3425,7 @@ sctp_disposition_t sctp_sf_do_asconf(const struct
> sctp_endpoint *ep,
> addr_param = (union sctp_addr_param *)hdr->params;
> length = ntohs(addr_param->p.length);
> if (length < sizeof(sctp_paramhdr_t))
> - return sctp_sf_violation_paramlen(ep, asoc, type,
> + return sctp_sf_violation_paramlen(ep, asoc, type, arg,
> (void *)addr_param, commands);
>
> /* Verify the ASCONF chunk before processing it. */
> @@ -3433,8 +3433,8 @@ sctp_disposition_t sctp_sf_do_asconf(const struct
> sctp_endpoint *ep,
> (sctp_paramhdr_t *)((void *)addr_param + length),
> (void *)chunk->chunk_end,
> &err_param))
> - return sctp_sf_violation_paramlen(ep, asoc, type,
> - (void *)&err_param, commands);
> + return sctp_sf_violation_paramlen(ep, asoc, type, arg,
> + (void *)err_param, commands);
>
> /* ADDIP 5.2 E1) Compare the value of the serial number to the value
> * the endpoint stored in a new association variable
> @@ -3542,8 +3542,8 @@ sctp_disposition_t sctp_sf_do_asconf_ack(const
> struct sctp_endpoint *ep,
> (sctp_paramhdr_t *)addip_hdr->params,
> (void *)asconf_ack->chunk_end,
> &err_param))
> - return sctp_sf_violation_paramlen(ep, asoc, type,
> - (void *)&err_param, commands);
> + return sctp_sf_violation_paramlen(ep, asoc, type, arg,
> + (void *)err_param, commands);
>
> if (last_asconf) {
> addip_hdr = (sctp_addiphdr_t *)last_asconf->subh.addip_hdr;
> @@ -4240,12 +4240,38 @@ static sctp_disposition_t
> sctp_sf_violation_paramlen(
> const struct sctp_endpoint *ep,
> const struct sctp_association *asoc,
> const sctp_subtype_t type,
> - void *arg,
> - sctp_cmd_seq_t *commands) {
> - static const char err_str[] = "The following parameter had invalid
> length:";
> + void *arg, void *ext,
> + sctp_cmd_seq_t *commands)
> +{
> + struct sctp_chunk *chunk = arg;
> + struct sctp_paramhdr *param = ext;
> + struct sctp_chunk *abort = NULL;
>
> - return sctp_sf_abort_violation(ep, asoc, arg, commands, err_str,
> - sizeof(err_str));
> + if (sctp_auth_recv_cid(SCTP_CID_ABORT, asoc))
> + goto discard;
> +
> + /* Make the abort chunk. */
> + abort = sctp_make_violation_paramlen(asoc, chunk, param);
> + if (!abort)
> + goto nomem;
> +
> + sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
> + SCTP_INC_STATS(SCTP_MIB_OUTCTRLCHUNKS);
> +
> + sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
> + SCTP_ERROR(ECONNABORTED));
> + sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
> + SCTP_PERR(SCTP_ERROR_PROTO_VIOLATION));
> + SCTP_DEC_STATS(SCTP_MIB_CURRESTAB);
> +
> +discard:
> + sctp_sf_pdiscard(ep, asoc, SCTP_ST_CHUNK(0), arg, commands);
> +
> + SCTP_INC_STATS(SCTP_MIB_ABORTEDS);
> +
> + return SCTP_DISPOSITION_ABORT;
> +nomem:
> + return SCTP_DISPOSITION_NOMEM;
> }
>
> /* Handle a protocol violation when the peer trying to advance the
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] sctp: Fix kernel panic while process protocol violation parameter
2008-09-08 1:15 [PATCH] sctp: Fix kernel panic while process protocol violation parameter Wei Yongjun
` (3 preceding siblings ...)
2008-09-19 20:14 ` Vlad Yasevich
@ 2008-09-25 21:14 ` Vlad Yasevich
2008-09-30 12:33 ` [PATCH] sctp: Fix kernel panic while process protocol David Miller
4 siblings, 1 reply; 7+ messages in thread
From: Vlad Yasevich @ 2008-09-25 21:14 UTC (permalink / raw)
To: davem; +Cc: linux-sctp, netdev, Wei Yongjun, Vlad Yasevich
From: Wei Yongjun <yjwei@cn.fujitsu.com>
Since call to function sctp_sf_abort_violation() need paramter 'arg' with
'struct sctp_chunk' type, it will read the chunk type and chunk length from
the chunk_hdr member of chunk. But call to sctp_sf_violation_paramlen()
always with 'struct sctp_paramhdr' type's parameter, it will be passed to
sctp_sf_abort_violation(). This may cause kernel panic.
sctp_sf_violation_paramlen()
|-- sctp_sf_abort_violation()
|-- sctp_make_abort_violation()
This patch fixed this problem. This patch also fix two place which called
sctp_sf_violation_paramlen() with wrong paramter type.
Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
---
include/net/sctp/sm.h | 3 ++
net/sctp/sm_make_chunk.c | 37 +++++++++++++++++++++++------------
net/sctp/sm_statefuns.c | 48 +++++++++++++++++++++++++++++++++++----------
3 files changed, 64 insertions(+), 24 deletions(-)
diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
index 2481173..029a54a 100644
--- a/include/net/sctp/sm.h
+++ b/include/net/sctp/sm.h
@@ -227,6 +227,9 @@ struct sctp_chunk *sctp_make_abort_violation(const struct sctp_association *,
const struct sctp_chunk *,
const __u8 *,
const size_t );
+struct sctp_chunk *sctp_make_violation_paramlen(const struct sctp_association *,
+ const struct sctp_chunk *,
+ struct sctp_paramhdr *);
struct sctp_chunk *sctp_make_heartbeat(const struct sctp_association *,
const struct sctp_transport *,
const void *payload,
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index fe94f42..1f882bb 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1012,6 +1012,29 @@ end:
return retval;
}
+struct sctp_chunk *sctp_make_violation_paramlen(
+ const struct sctp_association *asoc,
+ const struct sctp_chunk *chunk,
+ struct sctp_paramhdr *param)
+{
+ struct sctp_chunk *retval;
+ static const char error[] = "The following parameter had invalid length:";
+ size_t payload_len = sizeof(error) + sizeof(sctp_errhdr_t) +
+ sizeof(sctp_paramhdr_t);
+
+ retval = sctp_make_abort(asoc, chunk, payload_len);
+ if (!retval)
+ goto nodata;
+
+ sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION,
+ sizeof(error) + sizeof(sctp_paramhdr_t));
+ sctp_addto_chunk(retval, sizeof(error), error);
+ sctp_addto_param(retval, sizeof(sctp_paramhdr_t), param);
+
+nodata:
+ return retval;
+}
+
/* Make a HEARTBEAT chunk. */
struct sctp_chunk *sctp_make_heartbeat(const struct sctp_association *asoc,
const struct sctp_transport *transport,
@@ -1782,11 +1805,6 @@ static int sctp_process_inv_paramlength(const struct sctp_association *asoc,
const struct sctp_chunk *chunk,
struct sctp_chunk **errp)
{
- static const char error[] = "The following parameter had invalid length:";
- size_t payload_len = WORD_ROUND(sizeof(error)) +
- sizeof(sctp_paramhdr_t);
-
-
/* This is a fatal error. Any accumulated non-fatal errors are
* not reported.
*/
@@ -1794,14 +1812,7 @@ static int sctp_process_inv_paramlength(const struct sctp_association *asoc,
sctp_chunk_free(*errp);
/* Create an error chunk and fill it in with our payload. */
- *errp = sctp_make_op_error_space(asoc, chunk, payload_len);
-
- if (*errp) {
- sctp_init_cause(*errp, SCTP_ERROR_PROTO_VIOLATION,
- sizeof(error) + sizeof(sctp_paramhdr_t));
- sctp_addto_chunk(*errp, sizeof(error), error);
- sctp_addto_param(*errp, sizeof(sctp_paramhdr_t), param);
- }
+ *errp = sctp_make_violation_paramlen(asoc, chunk, param);
return 0;
}
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 8848d32..7c622af 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -119,7 +119,7 @@ static sctp_disposition_t sctp_sf_violation_paramlen(
const struct sctp_endpoint *ep,
const struct sctp_association *asoc,
const sctp_subtype_t type,
- void *arg,
+ void *arg, void *ext,
sctp_cmd_seq_t *commands);
static sctp_disposition_t sctp_sf_violation_ctsn(
@@ -3425,7 +3425,7 @@ sctp_disposition_t sctp_sf_do_asconf(const struct sctp_endpoint *ep,
addr_param = (union sctp_addr_param *)hdr->params;
length = ntohs(addr_param->p.length);
if (length < sizeof(sctp_paramhdr_t))
- return sctp_sf_violation_paramlen(ep, asoc, type,
+ return sctp_sf_violation_paramlen(ep, asoc, type, arg,
(void *)addr_param, commands);
/* Verify the ASCONF chunk before processing it. */
@@ -3433,8 +3433,8 @@ sctp_disposition_t sctp_sf_do_asconf(const struct sctp_endpoint *ep,
(sctp_paramhdr_t *)((void *)addr_param + length),
(void *)chunk->chunk_end,
&err_param))
- return sctp_sf_violation_paramlen(ep, asoc, type,
- (void *)&err_param, commands);
+ return sctp_sf_violation_paramlen(ep, asoc, type, arg,
+ (void *)err_param, commands);
/* ADDIP 5.2 E1) Compare the value of the serial number to the value
* the endpoint stored in a new association variable
@@ -3542,8 +3542,8 @@ sctp_disposition_t sctp_sf_do_asconf_ack(const struct sctp_endpoint *ep,
(sctp_paramhdr_t *)addip_hdr->params,
(void *)asconf_ack->chunk_end,
&err_param))
- return sctp_sf_violation_paramlen(ep, asoc, type,
- (void *)&err_param, commands);
+ return sctp_sf_violation_paramlen(ep, asoc, type, arg,
+ (void *)err_param, commands);
if (last_asconf) {
addip_hdr = (sctp_addiphdr_t *)last_asconf->subh.addip_hdr;
@@ -4240,12 +4240,38 @@ static sctp_disposition_t sctp_sf_violation_paramlen(
const struct sctp_endpoint *ep,
const struct sctp_association *asoc,
const sctp_subtype_t type,
- void *arg,
- sctp_cmd_seq_t *commands) {
- static const char err_str[] = "The following parameter had invalid length:";
+ void *arg, void *ext,
+ sctp_cmd_seq_t *commands)
+{
+ struct sctp_chunk *chunk = arg;
+ struct sctp_paramhdr *param = ext;
+ struct sctp_chunk *abort = NULL;
- return sctp_sf_abort_violation(ep, asoc, arg, commands, err_str,
- sizeof(err_str));
+ if (sctp_auth_recv_cid(SCTP_CID_ABORT, asoc))
+ goto discard;
+
+ /* Make the abort chunk. */
+ abort = sctp_make_violation_paramlen(asoc, chunk, param);
+ if (!abort)
+ goto nomem;
+
+ sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
+ SCTP_INC_STATS(SCTP_MIB_OUTCTRLCHUNKS);
+
+ sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
+ SCTP_ERROR(ECONNABORTED));
+ sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
+ SCTP_PERR(SCTP_ERROR_PROTO_VIOLATION));
+ SCTP_DEC_STATS(SCTP_MIB_CURRESTAB);
+
+discard:
+ sctp_sf_pdiscard(ep, asoc, SCTP_ST_CHUNK(0), arg, commands);
+
+ SCTP_INC_STATS(SCTP_MIB_ABORTEDS);
+
+ return SCTP_DISPOSITION_ABORT;
+nomem:
+ return SCTP_DISPOSITION_NOMEM;
}
/* Handle a protocol violation when the peer trying to advance the
--
1.5.3.5
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] sctp: Fix kernel panic while process protocol
2008-09-25 21:14 ` [PATCH] sctp: Fix kernel panic while process protocol violation parameter Vlad Yasevich
@ 2008-09-30 12:33 ` David Miller
0 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2008-09-30 12:33 UTC (permalink / raw)
To: vladislav.yasevich; +Cc: linux-sctp, netdev, yjwei
From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Thu, 25 Sep 2008 17:14:20 -0400
> From: Wei Yongjun <yjwei@cn.fujitsu.com>
>
> Since call to function sctp_sf_abort_violation() need paramter 'arg' with
> 'struct sctp_chunk' type, it will read the chunk type and chunk length from
> the chunk_hdr member of chunk. But call to sctp_sf_violation_paramlen()
> always with 'struct sctp_paramhdr' type's parameter, it will be passed to
> sctp_sf_abort_violation(). This may cause kernel panic.
>
> sctp_sf_violation_paramlen()
> |-- sctp_sf_abort_violation()
> |-- sctp_make_abort_violation()
>
> This patch fixed this problem. This patch also fix two place which called
> sctp_sf_violation_paramlen() with wrong paramter type.
>
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
Applied to net-2.6, thanks.
^ permalink raw reply [flat|nested] 7+ messages in thread