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: <CAFBinCC9sNvQJcu-SOSrFmo4sCx29K6KwXnc-O6MX9TJEHtXYg@mail.gmail.com>
Date:   Wed, 4 Jan 2023 17:07:40 +0100
From:   Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To:     David Laight <David.Laight@...lab.com>
Cc:     Ping-Ke Shih <pkshih@...ltek.com>,
        "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

On Wed, Jan 4, 2023 at 4:53 PM David Laight <David.Laight@...lab.com> wrote:
>
> 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).
My understanding is that there's one actual and one potential use-case.
Let's start with the actual one in
drivers/net/wireless/realtek/rtw88/rtw8821c.h:
  struct rtw8821c_efuse {
      __le16 rtl_id;
      u8 res0[0x0e];
      ...

The second one is a potential one, also in
drivers/net/wireless/realtek/rtw88/rtw8821c.h if we replace the
bitfields by an __le16 (which is my understanding how the data is
modeled in the eFuse):
  struct rtw8821ce_efuse {
      ...
      u8 serial_number[8];
      __le16 cap_data; /* 0xf4 */
      ...
(I'm not sure about the "cap_data" name, but I think you get the point)

> Possibly it should be 'u8 data2[2];'
So you're saying we should replace the __le16 with u8 some_name[2];
instead, then we don't need the __packed attribute.

> What you may want to do is add compile-time asserts for the
> sizes of the structures.
Do I get you right that something like:
  BUILD_BUG_ON(sizeof(rtw8821c_efuse) != 256);
is what you have in mind?



Best regards,
Martin