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

Date:   Mon, 8 Jun 2020 21:13:15 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Michael Tuexen' <Michael.Tuexen@...chi.franken.de>
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

From: Michael Tuexen
> Sent: 08 June 2020 18:37
> > 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.

Actually, what happens when the misaligned 'struct sockaddr'
(in the sctp options) is passed through to a function
that expects it to be aligned and then accesses part of (say)
an IPv6 structure using 8 bytes accesses.
That will 'crash and burn' on sparc64 as well.

> > 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.

Basically you should always have the address length.
I just remember Christos complaining about some kernel code
that allocated one on stack.
(My NetBSD 'commit bit' has rather lapsed.)

	David

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