Re: [PATCH net-next 3/8] sctp: implement validate_ftsn for sctp_stream_interleave

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Tue, Dec 12, 2017 at 09:31:36PM +0800, Xin Long wrote:
> On Tue, Dec 12, 2017 at 9:18 PM, Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> > On Tue, Dec 12, 2017 at 05:25:54PM +0800, Xin Long wrote:
> > ...
> >> --- a/net/sctp/sm_statetable.c
> >> +++ b/net/sctp/sm_statetable.c
> >> @@ -992,7 +992,8 @@ static const struct sctp_sm_table_entry *sctp_chunk_event_lookup(
> >>               return &chunk_event_table[cid][state];
> >>
> >>       if (net->sctp.prsctp_enable) {
> >> -             if (cid = SCTP_CID_FWD_TSN)
> >> +             if (cid = SCTP_CID_FWD_TSN ||
> >> +                 (net->sctp.intl_enable && cid = SCTP_CID_I_FWD_TSN))
> >
> > We don't really need to check intl_enable here, do we?
> We think net->sctp.xxx_enable as a main switch here,
> that means it only accepts SCTP_CID_I_FWD_TSN
> when this switch is open.

Yes, but

> 
> As for the check in validate_ftsn(), it follows the RFC checking
> on asoc's intl_enable.

Yes.

> 
> does it make sense from this perspective ?

Sort of. We need consistency. If we want do discard such invalid
chunks right here, that condition also needs to ensure that fwd_tsn
chunks are actually allowed (IOW, that intl_enable is false).

And we don't do any check for idata chunks. We just just accept them
and discard later when trying to consume it.

> 
> 
> 
> > Because a) it should actually look like:
> > -               if (cid = SCTP_CID_FWD_TSN)
> > +               if ((!net->sctp.intl_enable && cid = SCTP_CID_FWD_TSN) ||
> > +                   (net->sctp.intl_enable && cid = SCTP_CID_I_FWD_TSN))
> > but b) we will validate the chunk format/feature later with
> > validate_ftsn(), similarly to what happens with data chunks, so it
> > seems the check on intl_enable here is not necessary.
> >
> > Same happens with data chunks, btw.
> >
> >>                       return &prsctp_chunk_event_table[0][state];
> >>       }
> >>
> >> diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c
> >> index 2ead372..cc4a5e3 100644
> >> --- a/net/sctp/stream_interleave.c
> >> +++ b/net/sctp/stream_interleave.c
> >> @@ -1153,8 +1153,49 @@ static void sctp_generate_iftsn(struct sctp_outq *q, __u32 ctsn)
> >>       }
> >>  }
> >>
> >> +#define _sctp_walk_ifwdtsn(pos, chunk, end) \
> >> +     for (pos = chunk->subh.ifwdtsn_hdr->skip; \
> >> +          (void *)pos < (void *)chunk->subh.ifwdtsn_hdr->skip + (end); pos++)
> >> +
> >> +#define sctp_walk_ifwdtsn(pos, ch) \
> >> +     _sctp_walk_ifwdtsn((pos), (ch), ntohs((ch)->chunk_hdr->length) - \
> >> +                                     sizeof(struct sctp_ifwdtsn_chunk))
> >> +
> >> +static bool sctp_validate_fwdtsn(struct sctp_chunk *chunk)
> >> +{
> >> +     struct sctp_fwdtsn_skip *skip;
> >> +     __u16 incnt;
> >> +
> >> +     if (chunk->chunk_hdr->type != SCTP_CID_FWD_TSN)
> >> +             return false;
> >> +
> >> +     incnt = chunk->asoc->stream.incnt;
> >> +     sctp_walk_fwdtsn(skip, chunk)
> >> +             if (ntohs(skip->stream) >= incnt)
> >> +                     return false;
> >> +
> >> +     return true;
> >> +}
> >> +
> >> +static bool sctp_validate_iftsn(struct sctp_chunk *chunk)
> >> +{
> >> +     struct sctp_ifwdtsn_skip *skip;
> >> +     __u16 incnt;
> >> +
> >> +     if (chunk->chunk_hdr->type != SCTP_CID_I_FWD_TSN)
> >> +             return false;
> >> +
> >> +     incnt = chunk->asoc->stream.incnt;
> >> +     sctp_walk_ifwdtsn(skip, chunk)
> >> +             if (ntohs(skip->stream) >= incnt)
> >> +                     return false;
> >> +
> >> +     return true;
> >> +}
> >> +
> >>  static struct sctp_stream_interleave sctp_stream_interleave_0 = {
> >>       .data_chunk_len         = sizeof(struct sctp_data_chunk),
> >> +     .ftsn_chunk_len         = sizeof(struct sctp_fwdtsn_chunk),
> >>       /* DATA process functions */
> >>       .make_datafrag          = sctp_make_datafrag_empty,
> >>       .assign_number          = sctp_chunk_assign_ssn,
> >> @@ -1166,10 +1207,12 @@ static struct sctp_stream_interleave sctp_stream_interleave_0 = {
> >>       .abort_pd               = sctp_ulpq_abort_pd,
> >>       /* FORWARD-TSN process functions */
> >>       .generate_ftsn          = sctp_generate_fwdtsn,
> >> +     .validate_ftsn          = sctp_validate_fwdtsn,
> >>  };
> >>
> >>  static struct sctp_stream_interleave sctp_stream_interleave_1 = {
> >>       .data_chunk_len         = sizeof(struct sctp_idata_chunk),
> >> +     .ftsn_chunk_len         = sizeof(struct sctp_ifwdtsn_chunk),
> >>       /* I-DATA process functions */
> >>       .make_datafrag          = sctp_make_idatafrag_empty,
> >>       .assign_number          = sctp_chunk_assign_mid,
> >> @@ -1181,6 +1224,7 @@ static struct sctp_stream_interleave sctp_stream_interleave_1 = {
> >>       .abort_pd               = sctp_intl_abort_pd,
> >>       /* I-FORWARD-TSN process functions */
> >>       .generate_ftsn          = sctp_generate_iftsn,
> >> +     .validate_ftsn          = sctp_validate_iftsn,
> >>  };
> >>
> >>  void sctp_stream_interleave_init(struct sctp_stream *stream)
> >> --
> >> 2.1.0
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>