netdev - RE: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

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: <ec6a0988f3f943128e0122d50959185a@AcuMS.aculab.com>
Date:   Wed, 4 Jan 2023 15:53:17 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Martin Blumenstingl' <martin.blumenstingl@...glemail.com>,
        Ping-Ke Shih <pkshih@...ltek.com>
CC:     "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        "kvalo@...nel.org" <kvalo@...nel.org>,
        "tehuang@...ltek.com" <tehuang@...ltek.com>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "tony0620emma@...il.com" <tony0620emma@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

From: Martin Blumenstingl
> Sent: 04 January 2023 15:30
> 
> Hi Ping-Ke, Hi David,
> 
> On Sun, Jan 1, 2023 at 2:09 PM Ping-Ke Shih <pkshih@...ltek.com> wrote:
> [...]
> > Yes, it should not use bit filed. Instead, use a __le16 for all fields, such as
> I think this can be done in a separate patch.
> My v2 of this patch has reduced these changes to a minimum, see [0]
> 
> [...]
> > struct rtw8821ce_efuse {
> >    ...
> >    u8 data1;       // offset 0x100
> >    __le16 data2;   // offset 0x101-0x102
> >    ...
> > } __packed;
> >
> > Without __packed, compiler could has pad between data1 and data2,
> > and then get wrong result.
> My understanding is that this is the reason why we need __packed.

True, but does it really have to look like that?
I can't find that version (I don't have a net_next tree).
Possibly it should be 'u8 data2[2];'

Most hardware definitions align everything.

What you may want to do is add compile-time asserts for the
sizes of the structures.

Remember that if you have 16/32 bit fields in packed structures
on some architectures the compile has to generate code that does
byte loads and shifts.

The 'misaligned' property is lost when you take the address - so
you can easily generate a fault.

Adding __packed to a struct is a sledgehammer you really shouldn't need.

	David

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