netdev - Re: [PATCH net-next v6 23/23] net: WireGuard secure network tunnel

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: <20180927011526.GB1193@lunn.ch>
Date:   Thu, 27 Sep 2018 03:15:26 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        linux-crypto@...r.kernel.org, davem@...emloft.net,
        gregkh@...uxfoundation.org
Subject: Re: [PATCH net-next v6 23/23] net: WireGuard secure network tunnel

Hi Jason

I know you have been concentrating on the crypto code, so i'm not
expecting too many changes at the moment in the network code.

It would be good to further reduce the number of checkpatch warnings.
The biggest problem i have at the moment is that all the trivial to
fix warnings are hiding a few more important warnings. There are a few
like these which are not obvious to see:

WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
#2984: FILE: drivers/net/wireguard/noise.c:293:
+       BUG_ON(first_len > BLAKE2S_HASH_SIZE || second_len > BLAKE2S_HASH_SIZE ||

WARNING: Macros with flow control statements should be avoided
#5471: FILE: drivers/net/wireguard/selftest/allowedips.h:456:
+#define init_peer(name) do {                                               \
+               name = kzalloc(sizeof(*name), GFP_KERNEL);                 \
+               if (unlikely(!name)) {                                     \
+                       pr_info("allowedips self-test: out of memory\n");  \
+                       goto free;                                         \
+               }                                                          \
+               kref_init(&name->refcount);                                \
+       } while (0)


The namespace pollution also needs to be addresses. You have some
pretty generic named global symbols. I picked out a few examples from
objdump

00002a94 g     F .text	    00000060 peer_put
00003484 g     F .text	    0000004c timers_stop
00003520 g     F .text	    00000114 packet_queue_init
00002640 g     F .text	    00000034 device_uninit
000026bc g     F .text	    00000288 peer_create
000090d4 g     F .text	    000001bc ratelimiter_init

Please make use of a prefix for global symbols, e.g. wg_.

       Andrew