netdev - Re: [net-next PATCH] net: codel: Avoid undefined behavior from signed overflow

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: <20131031225332.38534539@redhat.com>
Date:	Thu, 31 Oct 2013 22:53:32 +0100
From:	Jesper Dangaard Brouer <jbrouer@...hat.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	Jesper Dangaard Brouer <brouer@...hat.com>,
	<netdev@...r.kernel.org>, Eric Dumazet <eric.dumazet@...il.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Dave Taht <dave.taht@...il.com>,
	Eilon Greenstein <eilong@...adcom.com>
Subject: Re: [net-next PATCH] net: codel: Avoid undefined behavior from
 signed overflow

On Wed, 30 Oct 2013 19:35:48 +0000
Ben Hutchings <bhutchings@...arflare.com> wrote:

> On Wed, 2013-10-30 at 18:23 +0100, Jesper Dangaard Brouer wrote:
> > From: Jesper Dangaard Brouer <netoptimizer@...uer.com>
> > 
> > As described in commit 5a581b367 (jiffies: Avoid undefined
> > behavior from signed overflow), according to the C standard
> > 3.4.3p3, overflow of a signed integer results in undefined
> > behavior.
> [...]
> 
> According to the real processors that Linux runs on, signed arithmetic
> uses 2's complement representation and overflow wraps accordingly.  And
> we rely on that behaviour in many places, so we use
> '-fno-strict-overflow' to tell gcc not to assume we avoid signed
> overflow.  (There is also '-fwrapv' which tells gcc to assume the
> processor behaves this way, but shouldn't it already know how the target
> machine works?)

For 16-bit I have tested that is fails, and that it does not help to
use the compiler flag: '-fno-strict-overflow' or '-fwrapv'. (this was
userspace test code, so I might be missing some kernel compiler options
that would make this work for 16-bit, but I doubt it)

#define works_u16_time_after(a,b)			\
	(typecheck(u_int16_t, a) &&		\
	 typecheck(u_int16_t, b) &&		\
	 ((int16_t)((b) - (a)) < 0))

#define bad_u16_time_after(a,b)			\
	(typecheck(u_int16_t, a) &&		\
	 typecheck(u_int16_t, b) &&		\
	 (((int16_t)(b) - (int16_t)(a)) < 0))


The bnx2x have a wrong/dangerup construct:

 File: drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
 #define SUB_S16(a, b)		(s16)((s16)(a) - (s16)(b))
 #define SUB_S32(a, b)		(s32)((s32)(a) - (s32)(b))

I have tested this case, and it surprisingly works, due to the outer
(s16) cast I believe.

I think this should/could be fixed like:

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
index 4e01c57..8969733 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
@@ -838,8 +838,8 @@ static inline bool bnx2x_fp_ll_polling(struct bnx2x_fastpath *fp)
 #define RCQ_TH_HI(bp)          (RCQ_TH_LO(bp) + DROPLESS_FC_HEADROOM)
 
 /* This is needed for determining of last_max */
-#define SUB_S16(a, b)          (s16)((s16)(a) - (s16)(b))
-#define SUB_S32(a, b)          (s32)((s32)(a) - (s32)(b))
+#define SUB_S16(a, b)          (s16)((u16)(a) - (u16)(b))
+#define SUB_S32(a, b)          (s32)((u32)(a) - (u32)(b))
 
 #define BNX2X_SWCID_SHIFT      17
 #define BNX2X_SWCID_MASK       ((0x1 << BNX2X_SWCID_SHIFT) - 1)


-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html