netdev - Re: [PATCH v2 1/3] sctp: Export struct sctp

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] [day] [month] [year] [list]

Message-ID: <CADvbK_dEm1EkkAxwu7GdceeQ5uDieWCM8KG4rXfBzbvPrp=oGg@mail.gmail.com>
Date:	Thu, 4 Aug 2016 20:16:10 +0800
From:	Xin Long <lucien.xin@...il.com>
To:	Phil Sutter <phil@....cc>, David Laight <David.Laight@...lab.com>,
	David Miller <davem@...emloft.net>,
	Xin Long <lucien.xin@...il.com>,
	Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 1/3] sctp: Export struct sctp_info to userspace

On Thu, Aug 4, 2016 at 5:27 PM, Phil Sutter <phil@....cc> wrote:
> On Thu, Aug 04, 2016 at 09:13:03AM +0000, David Laight wrote:
>> From: Phil Sutter
>> > Sent: 03 August 2016 22:23
>> > This is required to correctly interpret INET_DIAG_INFO messages exported
>> > by sctp_diag module.
>> ...
>> > diff --git a/include/linux/sctp.h b/include/linux/sctp.h
>> > index de1f64318fc4e..fcb4c36461732 100644
>> > --- a/include/linux/sctp.h
>> > +++ b/include/linux/sctp.h
>> > @@ -705,70 +705,6 @@ typedef struct sctp_auth_chunk {
>> >     sctp_authhdr_t auth_hdr;
>> >  } __packed sctp_auth_chunk_t;
>> >
>> > -struct sctp_info {
>> > -   __u32   sctpi_tag;
>> ...
>> > -   __u32   __reserved3;
>> > -};
>> > -
>> >  struct sctp_infox {
>> >     struct sctp_info *sctpinfo;
>> >     struct sctp_association *asoc;
>> > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
>> > index d304f4c9792c4..a406adcc0793e 100644
>> > --- a/include/uapi/linux/sctp.h
>> > +++ b/include/uapi/linux/sctp.h
>> > @@ -944,4 +944,68 @@ struct sctp_default_prinfo {
>> >     __u16 pr_policy;
>> >  };
>> >
>> > +struct sctp_info {
>> > +   __u32   sctpi_tag;
>>
>> Should these be uint32_t (etc) for userspace?
>
> Grepping through include/uapi in my clone of net-next, I see 271 results
> for uint32_t but 4595 ones for __u32. So while not necessarily correct,
> it seems to be the far more popular choice. Do you see any benefit in
> using the uint*_t typedefs instead?
>
>> > +   __u32   sctpi_state;
>> ...
>> > +   __u16   __reserved1;
>>
>> Is it worth adding some extra pad here in case anything extra needs
>> to be added to this set of data?
>>
>> ...
>> > +   __u32   __reserved3;
>>
>> Think I'd definitely add a few words of pad here.
>> Or at least make absolutely sure the interface passes the buffer length and
>> allows for kernels that report different length buffers.
>
> I merely copy and pasted the struct from include/linux/sctp.h without
> thinking about it's layout. Xin, what are your thoughts about this?
You can see all the __reserved* fields here are to fill the memory holes.
As sctp's asoc is a quite big structure, now we cannot know exactly
all we really should dump. It's very possible to add more fields here in
the future.  Think about not to break the compatibility with iproute at
that time, we will use __reserved* fields firstly, secondly put it to the
end of this structure. by now we're not sure what they will be.

>
> Thanks, Phil