netdev - Re: [endianness bug] cxgb4: mk_act_open_req() buggers ->{local,peer}

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: <20180817180949.GE6515@ZenIV.linux.org.uk>
Date:   Fri, 17 Aug 2018 19:09:49 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Ganesh Goudar <ganeshgr@...lsio.com>
Cc:     Rahul Lakkireddy <rahul.lakkireddy@...lsio.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [endianness bug] cxgb4: mk_act_open_req() buggers
 ->{local,peer}_ip on big-endian hosts

On Fri, Aug 17, 2018 at 04:43:20PM +0100, Al Viro wrote:
> On Fri, Aug 17, 2018 at 06:35:41PM +0530, Ganesh Goudar wrote:
> > Thanks, Al. The patch looks good to me but it does not seem
> > to be showing up in patchwork, should I resend the patch on
> > your behalf to net tree ?
> 
> Umm...  I thought net-next had been closed until -rc1, hadn't
> it?
> 
> Anyway, endianness cleanups and fixes of drivers/net/ethernet/chelsio
> can be found in vfs.git #net-endian.chelsio; I was planning to post
> that stuff on netdev after -rc1, but I would certainly appreciate
> a look from somebody familiar with the hardware prior to that, assuming
> you have time for that at the moment...  The stuff in there (it's
> based off net/master):
>       struct cxgb4_next_header .match_val/.match_mask/mask should be net-endian
>       cxgb4: fix TC-PEDIT-related parts of cxgb4_tc_flower on big-endian
>       cxgb4_tc_u32: trivial endianness annotations
>       cxgb4: trivial endianness annotations
>       libcxgb: trivial __percpu annotations
>       libcxgb: trivial endianness annotations
>       cxgb3: trivial endianness annotations
>       cxgb3: don't get cute with !! and shifts in t3_prep_adapter()...
>       [investigate][endianness bug] cxgb3: assigning htonl(11-bit value) to __be16 field is wrong
>       cxgb: trivial endianness annotations

... and updated for some of today's catch (== stuff caught while finding the
reply).  Another very likely bug hidden by force-casts: in t4_fwcache()
you put host-endian 0 or 1 into __be32 field, hiding that by force-cast.
Then feed that to hardware, which, judging by everything else nearby
expects big-endian there.  The only reason it works, AFAICS, is that you
only pass it FW_PARAM_DEV_FWCACHE_FLUSH (== 0); as soon as you get
a caller passing FW_PARAM_DEV_FWCACHE_FLUSHINV, you'll get breakage.

And frankly, comments like
                        while (count) {
                                /* the (__force u64) is because the compiler
                                 * doesn't understand the endian swizzling
                                 * going on
                                 */
                                writeq((__force u64)*src, dst);
                                src++;
                                dst++;
                                count--;
                        }
are more than slightly terrifying - piss on compiler, what about reviewers?  And
the authors themselves, for that matter...  FWIW, see the dmr's comments on
"you are not expected to understand that" story (bell labs site is buggered,
but wayback machine has it on
https://web.archive.org/web/20140724213028/http://cm.bell-labs.com/cm/cs/who/dmr/odd.html)
Especially the "The real problem is that we didn't understand what was going on either"
part...

Re that code - are you sure it doesn't need le64_to_cpu(*src)?  Because from what
I understand about PCI (which matches just fine to the comments in the same driver),
you probably do need that.  Again, the only real way to find out is to test on
big-endian host...