| 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
| ||
|
Message-ID: <20180814212524.GT6515@ZenIV.linux.org.uk>
Date: Tue, 14 Aug 2018 22:25:24 +0100
From: Al Viro <viro@...IV.linux.org.uk>
To: Rahul Lakkireddy <rahul.lakkireddy@...lsio.com>
Cc: ganeshgr@...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
How can cxgb4/cxgb4_tc_flower.c handling of 16bit
fields possibly work on b-e? Look:
case TCA_PEDIT_KEY_EX_HDR_TYPE_TCP:
switch (offset) {
case PEDIT_TCP_SPORT_DPORT:
if (~mask & PEDIT_TCP_UDP_SPORT_MASK)
offload_pedit(fs, cpu_to_be32(val) >> 16,
cpu_to_be32(mask) >> 16,
TCP_SPORT);
OK, we are feeding two results of >> 16 (i.e. the values in
range 0..65535 from the host POV) to offload_pedit(). Which does
static void offload_pedit(struct ch_filter_specification *fs, u32 val, u32 mask,
u8 field)
{
u32 set_val = val & ~mask;
OK, it's a value in range 0..65535.
u32 offset = 0;
u8 size = 1;
int i;
for (i = 0; i < ARRAY_SIZE(pedits); i++) {
if (pedits[i].field == field) {
go until we finally find this:
PEDIT_FIELDS(TCP_, SPORT, 2, nat_fport, 0),
i.e.
{TCP_SPORT, 2, offsetof(struct ch_filter_specification, nat_fport)}
offset = pedits[i].offset;
size = pedits[i].size;
... resulting in offset = offsetof(..., nat_fport), size = 2
break;
}
}
memcpy((u8 *)fs + offset, &set_val, size);
... and we copy the first two bytes of set_val to fs->nat_fport, right?
On little-endian, assuming that val & 0xffff was 256 * V0 + V1 and
mask & 0xffff - 256 * M0 + M1, we get cpu_to_be32(val) >> 16 equal to
256 * V1 + V0, and similar for mask, resuling in set_val containing
{V0 & ~M0, V1 & ~M1, 0, 0}, with the first two bytes copied to fs->nat_fport.
Now, think what will happen on big-endian. The value in set_val has upper
16 bits all zero, no matter what - shift anything 32bit down by 16 and you'll
get that. And on big-endian that's first two bytes of memory representation,
so this memcpy() is absolutely guaranteed to set fs->nat_fport to zero.
No matter how fancy the hardware is, it can't guess what had the other two
bytes been - CPU has discarded those before the NIC had a chance to see
them.
Am I right assuming that the val is supposed to be {S1, S0, D1, D0},
with sport == S1 * 256 + S0, dport == D1 * 256 + D0? If so, the following
ought to work [== COMPLETELY UNTESTED, in other words] on l-e same as the
current code does and do the right thing on b-e. Objections?
offload_pedit() is broken for big-endian; it's actually easier to spell the
memcpy (and in case of ports - memcpy-with-byteswap) explicitly, avoiding
both the b-e problems and getting rid of a lot of LoC, including an unpleasant
macro.
Signed-off-by: Al Viro <viro@...iv.linux.org.uk>
---
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
index 3db969eefba9..020ca0121fb4 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
@@ -43,27 +43,6 @@
#define STATS_CHECK_PERIOD (HZ / 2)
-static struct ch_tc_pedit_fields pedits[] = {
- PEDIT_FIELDS(ETH_, DMAC_31_0, 4, dmac, 0),
- PEDIT_FIELDS(ETH_, DMAC_47_32, 2, dmac, 4),
- PEDIT_FIELDS(ETH_, SMAC_15_0, 2, smac, 0),
- PEDIT_FIELDS(ETH_, SMAC_47_16, 4, smac, 2),
- PEDIT_FIELDS(IP4_, SRC, 4, nat_fip, 0),
- PEDIT_FIELDS(IP4_, DST, 4, nat_lip, 0),
- PEDIT_FIELDS(IP6_, SRC_31_0, 4, nat_fip, 0),
- PEDIT_FIELDS(IP6_, SRC_63_32, 4, nat_fip, 4),
- PEDIT_FIELDS(IP6_, SRC_95_64, 4, nat_fip, 8),
- PEDIT_FIELDS(IP6_, SRC_127_96, 4, nat_fip, 12),
- PEDIT_FIELDS(IP6_, DST_31_0, 4, nat_lip, 0),
- PEDIT_FIELDS(IP6_, DST_63_32, 4, nat_lip, 4),
- PEDIT_FIELDS(IP6_, DST_95_64, 4, nat_lip, 8),
- PEDIT_FIELDS(IP6_, DST_127_96, 4, nat_lip, 12),
- PEDIT_FIELDS(TCP_, SPORT, 2, nat_fport, 0),
- PEDIT_FIELDS(TCP_, DPORT, 2, nat_lport, 0),
- PEDIT_FIELDS(UDP_, SPORT, 2, nat_fport, 0),
- PEDIT_FIELDS(UDP_, DPORT, 2, nat_lport, 0),
-};
-
static struct ch_tc_flower_entry *allocate_flower_entry(void)
{
struct ch_tc_flower_entry *new = kzalloc(sizeof(*new), GFP_KERNEL);
@@ -306,81 +285,63 @@ static int cxgb4_validate_flow_match(struct net_device *dev,
return 0;
}
-static void offload_pedit(struct ch_filter_specification *fs, u32 val, u32 mask,
- u8 field)
-{
- u32 set_val = val & ~mask;
- u32 offset = 0;
- u8 size = 1;
- int i;
-
- for (i = 0; i < ARRAY_SIZE(pedits); i++) {
- if (pedits[i].field == field) {
- offset = pedits[i].offset;
- size = pedits[i].size;
- break;
- }
- }
- memcpy((u8 *)fs + offset, &set_val, size);
-}
-
-static void process_pedit_field(struct ch_filter_specification *fs, u32 val,
- u32 mask, u32 offset, u8 htype)
+static void process_pedit_field(struct ch_filter_specification *fs, __be32 val,
+ __be32 mask, u32 offset, u8 htype)
{
+ val &= ~mask;
switch (htype) {
case TCA_PEDIT_KEY_EX_HDR_TYPE_ETH:
switch (offset) {
case PEDIT_ETH_DMAC_31_0:
fs->newdmac = 1;
- offload_pedit(fs, val, mask, ETH_DMAC_31_0);
+ memcpy(fs->dmac, &val, 4);
break;
case PEDIT_ETH_DMAC_47_32_SMAC_15_0:
if (~mask & PEDIT_ETH_DMAC_MASK)
- offload_pedit(fs, val, mask, ETH_DMAC_47_32);
+ memcpy(fs->dmac + 4, &val, 2);
else
- offload_pedit(fs, val >> 16, mask >> 16,
- ETH_SMAC_15_0);
+ memcpy(fs->smac, (__be16 *)&val + 1, 2);
break;
case PEDIT_ETH_SMAC_47_16:
fs->newsmac = 1;
- offload_pedit(fs, val, mask, ETH_SMAC_47_16);
+ memcpy(fs->smac + 2, &val, 4);
}
break;
case TCA_PEDIT_KEY_EX_HDR_TYPE_IP4:
switch (offset) {
case PEDIT_IP4_SRC:
- offload_pedit(fs, val, mask, IP4_SRC);
+ memcpy(fs->nat_fip, &val, 4);
break;
case PEDIT_IP4_DST:
- offload_pedit(fs, val, mask, IP4_DST);
+ memcpy(fs->nat_lip, &val, 4);
}
fs->nat_mode = NAT_MODE_ALL;
break;
case TCA_PEDIT_KEY_EX_HDR_TYPE_IP6:
switch (offset) {
case PEDIT_IP6_SRC_31_0:
- offload_pedit(fs, val, mask, IP6_SRC_31_0);
+ memcpy(fs->nat_fip, &val, 4);
break;
case PEDIT_IP6_SRC_63_32:
- offload_pedit(fs, val, mask, IP6_SRC_63_32);
+ memcpy(fs->nat_fip + 4, &val, 4);
break;
case PEDIT_IP6_SRC_95_64:
- offload_pedit(fs, val, mask, IP6_SRC_95_64);
+ memcpy(fs->nat_fip + 8, &val, 4);
break;
case PEDIT_IP6_SRC_127_96:
- offload_pedit(fs, val, mask, IP6_SRC_127_96);
+ memcpy(fs->nat_fip + 12, &val, 4);
break;
case PEDIT_IP6_DST_31_0:
- offload_pedit(fs, val, mask, IP6_DST_31_0);
+ memcpy(fs->nat_lip, &val, 4);
break;
case PEDIT_IP6_DST_63_32:
- offload_pedit(fs, val, mask, IP6_DST_63_32);
+ memcpy(fs->nat_lip + 4, &val, 4);
break;
case PEDIT_IP6_DST_95_64:
- offload_pedit(fs, val, mask, IP6_DST_95_64);
+ memcpy(fs->nat_lip + 8, &val, 4);
break;
case PEDIT_IP6_DST_127_96:
- offload_pedit(fs, val, mask, IP6_DST_127_96);
+ memcpy(fs->nat_lip + 12, &val, 4);
}
fs->nat_mode = NAT_MODE_ALL;
break;
@@ -388,12 +349,9 @@ static void process_pedit_field(struct ch_filter_specification *fs, u32 val,
switch (offset) {
case PEDIT_TCP_SPORT_DPORT:
if (~mask & PEDIT_TCP_UDP_SPORT_MASK)
- offload_pedit(fs, cpu_to_be32(val) >> 16,
- cpu_to_be32(mask) >> 16,
- TCP_SPORT);
+ fs->nat_fport = be16_to_cpup((__be16 *)&val);
else
- offload_pedit(fs, cpu_to_be32(val),
- cpu_to_be32(mask), TCP_DPORT);
+ fs->nat_lport = be16_to_cpup((__be16 *)&val + 1);
}
fs->nat_mode = NAT_MODE_ALL;
break;
@@ -401,12 +359,9 @@ static void process_pedit_field(struct ch_filter_specification *fs, u32 val,
switch (offset) {
case PEDIT_UDP_SPORT_DPORT:
if (~mask & PEDIT_TCP_UDP_SPORT_MASK)
- offload_pedit(fs, cpu_to_be32(val) >> 16,
- cpu_to_be32(mask) >> 16,
- UDP_SPORT);
+ fs->nat_fport = be16_to_cpup((__be16 *)&val);
else
- offload_pedit(fs, cpu_to_be32(val),
- cpu_to_be32(mask), UDP_DPORT);
+ fs->nat_lport = be16_to_cpup((__be16 *)&val + 1);
}
fs->nat_mode = NAT_MODE_ALL;
}
@@ -453,7 +408,8 @@ static void cxgb4_process_flow_actions(struct net_device *in,
break;
}
} else if (is_tcf_pedit(a)) {
- u32 mask, val, offset;
+ __be32 mask, val;
+ u32 offset;
int nkeys, i;
u8 htype;
@@ -471,23 +427,18 @@ static void cxgb4_process_flow_actions(struct net_device *in,
}
}
-static bool valid_l4_mask(u32 mask)
+static bool valid_l4_mask(__be32 mask)
{
- u16 hi, lo;
-
- /* Either the upper 16-bits (SPORT) OR the lower
- * 16-bits (DPORT) can be set, but NOT BOTH.
+ /* Either the SPORT OR DPORT can be set, but NOT BOTH.
*/
- hi = (mask >> 16) & 0xFFFF;
- lo = mask & 0xFFFF;
-
- return hi && lo ? false : true;
+ return !(mask && htonl(0xffff)) || !(mask & htonl(0xffff0000));
}
static bool valid_pedit_action(struct net_device *dev,
const struct tc_action *a)
{
- u32 mask, offset;
+ __be32 mask;
+ u32 offset;
u8 cmd, htype;
int nkeys, i;
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h
index 050c8a50ae41..4da5267726a9 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h
@@ -54,44 +54,8 @@ struct ch_tc_flower_entry {
u32 filter_id;
};
-enum {
- ETH_DMAC_31_0, /* dmac bits 0.. 31 */
- ETH_DMAC_47_32, /* dmac bits 32..47 */
- ETH_SMAC_15_0, /* smac bits 0.. 15 */
- ETH_SMAC_47_16, /* smac bits 16..47 */
-
- IP4_SRC, /* 32-bit IPv4 src */
- IP4_DST, /* 32-bit IPv4 dst */
-
- IP6_SRC_31_0, /* src bits 0.. 31 */
- IP6_SRC_63_32, /* src bits 63.. 32 */
- IP6_SRC_95_64, /* src bits 95.. 64 */
- IP6_SRC_127_96, /* src bits 127..96 */
-
- IP6_DST_31_0, /* dst bits 0.. 31 */
- IP6_DST_63_32, /* dst bits 63.. 32 */
- IP6_DST_95_64, /* dst bits 95.. 64 */
- IP6_DST_127_96, /* dst bits 127..96 */
-
- TCP_SPORT, /* 16-bit TCP sport */
- TCP_DPORT, /* 16-bit TCP dport */
-
- UDP_SPORT, /* 16-bit UDP sport */
- UDP_DPORT, /* 16-bit UDP dport */
-};
-
-struct ch_tc_pedit_fields {
- u8 field;
- u8 size;
- u32 offset;
-};
-
-#define PEDIT_FIELDS(type, field, size, fs_field, offset) \
- { type## field, size, \
- offsetof(struct ch_filter_specification, fs_field) + (offset) }
-
-#define PEDIT_ETH_DMAC_MASK 0xffff
-#define PEDIT_TCP_UDP_SPORT_MASK 0xffff
+#define PEDIT_ETH_DMAC_MASK htonl(0xffff0000)
+#define PEDIT_TCP_UDP_SPORT_MASK htonl(0xffff0000)
#define PEDIT_ETH_DMAC_31_0 0x0
#define PEDIT_ETH_DMAC_47_32_SMAC_15_0 0x4
#define PEDIT_ETH_SMAC_47_16 0x8
Powered by blists - more mailing lists