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

Date:   Mon, 8 Jun 2020 08:17:05 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Michael Tuexen' <Michael.Tuexen@...chi.franken.de>
CC:     "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: 07 June 2020 22:51
> > On 7. Jun 2020, at 22:21, David Laight <David.Laight@...LAB.COM> wrote:
> >
> > From: Michael Tuexen
> >> Sent: 07 June 2020 18:24
> >>> On 7. Jun 2020, at 19:14, David Laight <David.Laight@...LAB.COM> wrote:
> >>>
> >>> From: Michael Tuexen <Michael.Tuexen@...chi.franken.de>
> >>>> Sent: 07 June 2020 16:15
> >>>>> On 7. Jun 2020, at 15:53, David Laight <David.Laight@...LAB.COM> wrote:
> >>>>>
> >>>>> From: Michael Tuexen
> >>>>>>
> >>>>>> since gcc uses -Werror=address-of-packed-member, I get warnings for my variant
> >>>>>> of packetdrill, which supports SCTP.
> >>>>>>
> >>>>>> Here is why:
> >>>>>>
> >>>>>>
> >>>>
> >>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/sctp.h?h=v5
> >>>>>> .7
> >>>>>> 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'm guessing 'to remove holes to avoid leaking kernel data'.
> >>>>>
> >>>>> The sctp socket api defines loads of structures that will have
> >>>>> holes in them if not packed.
> >>>>
> >>>> Hi David,
> >>>> I agree that they have holes and we should have done better. The
> >>>> kernel definitely should also not leak kernel data. However, the
> >>>> way to handle this shouldn't be packing. I guess it is too late
> >>>> to change this?
> >>>
> >>> Probably too late.
> >>> I've no idea how it got through the standards body either.
> >>> In fact, the standard may actually require the holes.
> >>
> >> No, it does not. Avoiding holes was not taken into account.
> >
> > It depends on whether the rfc that describes the sockops says
> > the structures 'look like this' or 'contain the following members'.
>
> It uses "is defined as"... Using "contain the following members"
> would have been a better way. But is wasn't used. So yes, we could
> have minimised the number of holes. But also other structure have
> them. So when passing them from kernel land to user land one has
> to zero out the padding. Not optimal, but doable.

If it says 'defined as' then the 'packed' is just wrong.

No idea what can be done about it.
But an application is within its rights to define the structure
as it is defined in the rfc.

	David

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