netdev - Re: [PATCHv3 net-next 2/7] sctp: add support for generating stream reconf ssn reset request chunk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 17 Jan 2017 12:27:48 +0800
From:   Xin Long <lucien.xin@...il.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc:     network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
        Neil Horman <nhorman@...driver.com>,
        Vlad Yasevich <vyasevich@...il.com>,
        davem <davem@...emloft.net>
Subject: Re: [PATCHv3 net-next 2/7] sctp: add support for generating stream
 reconf ssn reset request chunk

On Mon, Jan 16, 2017 at 11:56 AM, Xin Long <lucien.xin@...il.com> wrote:
> On Sun, Jan 15, 2017 at 11:51 PM, Marcelo Ricardo Leitner
> <marcelo.leitner@...il.com> wrote:
>> On Sat, Jan 14, 2017 at 03:15:36AM +0800, Xin Long wrote:
>>> This patch is to add asoc strreset_outseq and strreset_inseq for
>>> saving the reconf request sequence, initialize them when create
>>> assoc and process init, and also to define Incoming and Outgoing
>>> SSN Reset Request Parameter described in rfc6525 section 4.1 and
>>> 4.2, As they can be in one same chunk as section rfc6525 3.1-3
>>> describes, it makes them in one function.
>>>
>>> Signed-off-by: Xin Long <lucien.xin@...il.com>
>>> ---
>>>  include/linux/sctp.h       | 26 ++++++++++++++
>>>  include/net/sctp/sm.h      |  5 ++-
>>>  include/net/sctp/structs.h |  3 ++
>>>  net/sctp/associola.c       |  1 +
>>>  net/sctp/sm_make_chunk.c   | 88 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  5 files changed, 122 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/sctp.h b/include/linux/sctp.h
>>> index cdc3b05..d5da19c 100644
>>> --- a/include/linux/sctp.h
>>> +++ b/include/linux/sctp.h
>>> @@ -200,6 +200,13 @@ typedef enum {
>>>       SCTP_PARAM_SUCCESS_REPORT       = cpu_to_be16(0xc005),
>>>       SCTP_PARAM_ADAPTATION_LAYER_IND = cpu_to_be16(0xc006),
>>>
>>> +     /* RE-CONFIG. Section 4 */
>>> +     SCTP_PARAM_RESET_OUT_REQUEST            = cpu_to_be16(0x000d),
>>> +     SCTP_PARAM_RESET_IN_REQUEST             = cpu_to_be16(0x000e),
>>> +     SCTP_PARAM_RESET_TSN_REQUEST            = cpu_to_be16(0x000f),
>>> +     SCTP_PARAM_RESET_RESPONSE               = cpu_to_be16(0x0010),
>>> +     SCTP_PARAM_RESET_ADD_OUT_STREAMS        = cpu_to_be16(0x0011),
>>> +     SCTP_PARAM_RESET_ADD_IN_STREAMS         = cpu_to_be16(0x0012),
>>>  } sctp_param_t; /* enum */
>>>
>>>
>>> @@ -716,4 +723,23 @@ struct sctp_reconf_chunk {
>>>       __u8 params[0];
>>>  } __packed;
>>>
>>> +struct sctp_strreset_req {
>>> +     sctp_paramhdr_t param_hdr;
>>> +     __u32 request_seq;
>>> +} __packed;
>>> +
>>> +struct sctp_strreset_outreq {
>>> +     sctp_paramhdr_t param_hdr;
>>> +     __u32 request_seq;
>>
>> This should be:
>> +       struct sctp_strreset_req strreset_req;
>> Use the definition you created above for the encapsulation and make the
>> embedding evident.
>> Like it's done for sctp_chunkhdr_t.
> I'm not sure if it's good to do it like sctp_chunkhdr_t.
>
> As sctp_chunkhdr is a very common data:
> Chunk Type  | Chunk  Flags  |  Chunk Length
> and the next must be "Chunk Value"
>
> But here sctp_strreset_req is more used to access
> the request_seq of the params in asoc->strreset_chunk
> without knowing params' type. like in sctp_chunk_lookup_strreset_param()
> and sctp_process_strreset_resp() [see it from the big patchset].
>
> struct sctp_strreset_outreq {
>         sctp_paramhdr_t param_hdr;
>         __u32 request_seq;
>         ------------------------------------[1]
>         __u32 response_seq;
>         __u32 send_reset_at_tsn;
>         __u16 list_of_streams[0];
> } __packed;
>
> it seems not good to split it by [1].
>         __u32 request_seq;
>         __u32 response_seq;
>         __u32 send_reset_at_tsn;
>         __u16 list_of_streams[0];
> these should to together and equal.
>
> what do you think ?
>
Hi, Marcelo, I'm planning to drop "struct sctp_strreset_req" so
that it will not be confusing here, and only introduce it when
it's used somewhere. agreed ?