On 02/23/2016 01:49 PM, Jamal Hadi Salim wrote:
From: Jamal Hadi Salim <[email protected]>
[...]
+static const struct nla_policy ife_policy[TCA_IFE_MAX + 1] = {
+ [TCA_IFE_PARMS] = {.len = sizeof(struct tc_ife)},
+ [TCA_IFE_DMAC] = {.type = NLA_BINARY,.len = ETH_ALEN},
+ [TCA_IFE_SMAC] = {.type = NLA_BINARY,.len = ETH_ALEN},
This is buggy btw ...
+ [TCA_IFE_TYPE] = {.type = NLA_U16},
+};
[...]
+ if (parm->flags & IFE_ENCODE) {
+ ife_type = *(u16 *) nla_data(tb[TCA_IFE_TYPE]);
( We have accessors for such things. Please also check coding style
and white space things in your series, there's couple of things all
over the place. )
+ if (tb[TCA_IFE_DMAC] != NULL)
+ daddr = nla_data(tb[TCA_IFE_DMAC]);
+ if (tb[TCA_IFE_SMAC] != NULL)
+ saddr = nla_data(tb[TCA_IFE_SMAC]);
... NLA_BINARY with len means: max length. But there's nothing
that checks a min length on this, so you potentially access out
of bounds since everything <= ETH_ALEN is allowed in your case.
+ }
+
+ spin_lock_bh(&ife->tcf_lock);
Maybe try to make this lockless in the fast path? Otherwise placing
this on ingress / egress (f.e. clsact) doesn't really scale.
+ ife->tcf_action = parm->action;
+
+ if (parm->flags & IFE_ENCODE) {
+ if (daddr)
+ ether_addr_copy(ife->eth_dst, daddr);
+ else
+ eth_zero_addr(ife->eth_dst);
+
+ if (saddr)
+ ether_addr_copy(ife->eth_src, saddr);
+ else
+ eth_zero_addr(ife->eth_src);
+
+ ife->eth_type = ife_type;
+ }
