[PATCH 2/3] sctp:check parameter value of length in HEARTBEAT chunk

[PATCH 2/3] sctp:check parameter value of length in HEARTBEAT chunk

[Shan Wei]

When an endpoint receives HEARTBEAT that parameter value is invalid,
send an ABORT to peer with a Protocol Violation error code.

Signed-off-by: Shan Wei <shanwei@cn.fujitsu.com>
---
 net/sctp/sm_statefuns.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 3d3e36b..32e75ea 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -1067,6 +1067,10 @@ sctp_disposition_t sctp_sf_beat_8_3(const struct sctp_endpoint *ep,
 	 */
 	chunk->subh.hb_hdr = (sctp_heartbeathdr_t *) chunk->skb->data;
 	paylen = ntohs(chunk->chunk_hdr->length) - sizeof(sctp_chunkhdr_t);
+
+	if (ntohs(chunk->subh.hb_hdr->info.length) != paylen)
+		sctp_sf_violation_paramvalue(ep, asoc, type, arg,
+					     commands);
 	if (!pskb_pull(chunk->skb, paylen))
 		goto nomem;
 
-- 
1.6.3.3

Re: [PATCH 2/3] sctp:check parameter value of length in HEARTBEAT

[Vlad Yasevich]

Shan Wei wrote:
> When an endpoint receives HEARTBEAT that parameter value is invalid,
> send an ABORT to peer with a Protocol Violation error code.
> 
> Signed-off-by: Shan Wei <shanwei@cn.fujitsu.com>
> ---
>  net/sctp/sm_statefuns.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index 3d3e36b..32e75ea 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -1067,6 +1067,10 @@ sctp_disposition_t sctp_sf_beat_8_3(const struct sctp_endpoint *ep,
>  	 */
>  	chunk->subh.hb_hdr = (sctp_heartbeathdr_t *) chunk->skb->data;
>  	paylen = ntohs(chunk->chunk_hdr->length) - sizeof(sctp_chunkhdr_t);
> +
> +	if (ntohs(chunk->subh.hb_hdr->info.length) != paylen)
> +		sctp_sf_violation_paramvalue(ep, asoc, type, arg,
> +					     commands);

I don't think this is right as the parameter length doesn't account for the
padding, but the chunk length may.  Thus if such implementation sends us a HB,
we'll respond with an abort.

I don't see much point in this check.  HB parameters are opaque.  If someone
violates the protocol here, aborting the association is a very harsh treatment
since they are not really causing any overflows or any other conditions.

They may unnecessarily transmit horribly big HBs, but that's entirely up to them.

-vlad

>  	if (!pskb_pull(chunk->skb, paylen))
>  		goto nomem;
>