netdev - Re: packed structures used in socket options

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]

Message-Id: <7BD347D7-562F-459D-B0CB-0BC798919876@lurchi.franken.de>
Date:   Mon, 8 Jun 2020 19:37:25 +0200
From:   Michael Tuexen <Michael.Tuexen@...chi.franken.de>
To:     David Laight <David.Laight@...LAB.COM>
Cc:     Ivan Skytte Jørgensen <isj-sctp@...dk>,
        "linux-sctp@...r.kernel.org" <linux-sctp@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: packed structures used in socket options



> On 8. Jun 2020, at 18:18, David Laight <David.Laight@...LAB.COM> wrote:
> 
> From: Ivan Skytte Jørgensen
>> Sent: 07 June 2020 22:35
> ...
>>>>>>>> contains:
>>>>>>>> 
>>>>>>>> struct sctp_paddrparams {
>>>>>>>> 	sctp_assoc_t		spp_assoc_id;
>>>>>>>> 	struct sockaddr_storage	spp_address;
>>>>>>>> 	__u32			spp_hbinterval;
>>>>>>>> 	__u16			spp_pathmaxrxt;
>>>>>>>> 	__u32			spp_pathmtu;
>>>>>>>> 	__u32			spp_sackdelay;
>>>>>>>> 	__u32			spp_flags;
>>>>>>>> 	__u32			spp_ipv6_flowlabel;
>>>>>>>> 	__u8			spp_dscp;
>>>>>>>> } __attribute__((packed, aligned(4)));
>>>>>>>> 
>>>>>>>> This structure is only used in the IPPROTO_SCTP level socket option SCTP_PEER_ADDR_PARAMS.
>>>>>>>> Why is it packed?
> ...
>> I was involved. At that time (September 2005) the SCTP API was still evolving (first finalized in
>> 2011), and one of the major users of the API was 32-bit programs running on 64-bit kernel (on powerpc
>> as I recall). When we realized that the structures were different between 32bit and 64bit we had to
>> break the least number of programs, and the result were those ((packed)) structs so 32-bit programs
>> wouldn't be broken and we didn't need a xxx_compat translation layer in the kernel.
> 
> I was also looking at all the __u16 in that header - borked.
> 
> Ok, so the intention was to avoid padding caused by the alignment
> of sockaddr_storage rather than around the '__u16 spp_flags'.
> 
> I'd have to look up what (packed, aligned(4)) actually means.
> It could force the structure to be fully packed (no holes)
> but always have an overall alignment of 4.
> 
> It might have been clearer to put an 'aligned(4)' attribute
> on the spp_address field itself.
> Or even wonder whether sockaddr_storage should actually
> have 8 byte alignment.
> 
> If it has 16 byte alignment then you cannot cast an IPv4
> socket buffer address (which will be at most 4 byte aligned)
> to sockaddr_storage and expect the compiler not to generate
> code that will crash and burn on sparc64.
> 
> ISTR that the NetBSD view was that 'sockaddr_storage' should
> never actually be instantiated - it only existed as a typed
> pointer.
Not sure this is correct. I would say this applies to stuct sockaddr *.
I have seen instantiated sockaddr_storage variable in generic code,
where you need to provide enough space to hold an address, not yet
knowing the address family. However, I'm not familiar with the NetBSD
code base.

Best regards
Michael
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>