netdev - Re: [PATCH 13/17] staging: wfx: fix endianness of the field 'len'

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]

Message-ID: <1922297.S9MP3OIeVN@pc-42>
Date:   Tue, 12 May 2020 11:25:47 +0200
From:   Jérôme Pouiller <jerome.pouiller@...abs.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     driverdevel <devel@...verdev.osuosl.org>,
        linux-wireless <linux-wireless@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Kalle Valo <kvalo@...eaurora.org>,
        "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH 13/17] staging: wfx: fix endianness of the field 'len'

On Tuesday 12 May 2020 09:43:34 CEST Geert Uytterhoeven wrote:
> Hi Jerome,
> 
> On Mon, May 11, 2020 at 5:53 PM Jerome Pouiller
> <Jerome.Pouiller@...abs.com> wrote:
> > From: Jérôme Pouiller <jerome.pouiller@...abs.com>
> >
> > The struct hif_msg is received from the hardware. So, it declared as
> > little endian. However, it is also accessed from many places in the
> > driver. Sparse complains about that:
> >
> >     drivers/staging/wfx/bh.c:88:32: warning: restricted __le16 degrades to integer
> >     drivers/staging/wfx/bh.c:88:32: warning: restricted __le16 degrades to integer
> >     drivers/staging/wfx/bh.c:93:32: warning: restricted __le16 degrades to integer
> >     drivers/staging/wfx/bh.c:93:32: warning: cast to restricted __le16
> >     drivers/staging/wfx/bh.c:93:32: warning: restricted __le16 degrades to integer
> >     drivers/staging/wfx/bh.c:121:25: warning: incorrect type in argument 2 (different base types)
> >     drivers/staging/wfx/bh.c:121:25:    expected unsigned int len
> >     drivers/staging/wfx/bh.c:121:25:    got restricted __le16 [usertype] len
> >     drivers/staging/wfx/hif_rx.c:27:22: warning: restricted __le16 degrades to integer
> >     drivers/staging/wfx/hif_rx.c:347:39: warning: incorrect type in argument 7 (different base types)
> >     drivers/staging/wfx/hif_rx.c:347:39:    expected unsigned int [usertype] len
> >     drivers/staging/wfx/hif_rx.c:347:39:    got restricted __le16 const [usertype] len
> >     drivers/staging/wfx/hif_rx.c:365:39: warning: incorrect type in argument 7 (different base types)
> >     drivers/staging/wfx/hif_rx.c:365:39:    expected unsigned int [usertype] len
> >     drivers/staging/wfx/hif_rx.c:365:39:    got restricted __le16 const [usertype] len
> >     drivers/staging/wfx/./traces.h:195:1: warning: incorrect type in assignment (different base types)
> >     drivers/staging/wfx/./traces.h:195:1:    expected int msg_len
> >     drivers/staging/wfx/./traces.h:195:1:    got restricted __le16 const [usertype] len
> >     drivers/staging/wfx/./traces.h:195:1: warning: incorrect type in assignment (different base types)
> >     drivers/staging/wfx/./traces.h:195:1:    expected int msg_len
> >     drivers/staging/wfx/./traces.h:195:1:    got restricted __le16 const [usertype] len
> >     drivers/staging/wfx/debug.c:319:20: warning: restricted __le16 degrades to integer
> >     drivers/staging/wfx/secure_link.c:85:27: warning: restricted __le16 degrades to integer
> >     drivers/staging/wfx/secure_link.c:85:27: warning: restricted __le16 degrades to integer
> 
> Thanks for your patch!
> 
> > In order to make Sparse happy and to keep access from the driver easy,
> > this patch declare 'len' with native endianness.
> >
> > On reception of hardware data, this patch takes care to do byte-swap and
> > keep Sparse happy.
> 
> Which means sparse can no longer do any checking on the field,
> and new bugs may/will creep in in the future, unnoticed.
> 
> > --- a/drivers/staging/wfx/hif_api_general.h
> > +++ b/drivers/staging/wfx/hif_api_general.h
> > @@ -23,7 +23,10 @@
> >  #define HIF_COUNTER_MAX           7
> >
> >  struct hif_msg {
> > -       __le16 len;
> > +       // len is in fact little endian. However, it is widely used in the
> > +       // driver, so we declare it in native byte order and we reorder just
> > +       // before/after send/receive it (see bh.c).
> > +       u16    len;
> 
> While there's a small penalty associated with always doing the conversion
> on big-endian platforms, it will probably be lost in the noise anyway.

I have made the changes to show you that the code is far more complicated
with a le16... and the result was not as complicated as I expected...

I am going to post a v2.


-- 
Jérôme Pouiller