Le 11.09.2013 17:10, Or Gerlitz a écrit :
On 11/09/2013 17:04, Yann Droneaud wrote:

[...]
+       int i;
+       int kern_attr_size;
+
+       if (out_len < sizeof(resp))
+               return -ENOSPC;
+
+       if (copy_from_user(&cmd, buf, sizeof(cmd)))
+               return -EFAULT;
+
+       if (cmd.comp_mask)
+               return -EINVAL;
+
+       if ((cmd.flow_attr.type == IB_FLOW_ATTR_SNIFFER &&
+            !capable(CAP_NET_ADMIN)) || !capable(CAP_NET_RAW))
+               return -EPERM;
+
+       if (cmd.flow_attr.num_of_specs < 0 ||
num_of_specs is unsigned ...

sure, will send fix that eliminates this redundant check

+           cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
+               return -EINVAL;
+
+       kern_attr_size = cmd.flow_attr.size - sizeof(cmd) -
+                        sizeof(struct ib_uverbs_cmd_hdr_ex);
+
Please, no under/overflow. Check that cmd.flow_attr.size is bigger than sizeof(cmd) + sizeof(struct ib_uverbs_cmd_hdr_ex) before computing kern_attr_size.

We've got feedback on this code from Sean and Roland and Matan Barak
from our team addressed it to the bit.
Matan is OOO for couple of days, plus we're having few holidays here.
This way or another (accepting the comments
and sending fixes or arguing with you over the list), all your
feedback will be addressed before 3.12-rc2 is out, thanks for
looking over this!


Thanks a lot.


+       if (cmd.flow_attr.size < 0 || cmd.flow_attr.size > in_len ||
just like num_of_specs, it's unsigned !

ditto here, will fix.

[...]

diff --git a/include/uapi/rdma/ib_user_verbs.h
b/include/uapi/rdma/ib_user_verbs.h
index 61535aa..0b233c5 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -86,7 +86,9 @@ enum {

+struct ib_kern_spec {
+       union {
+               struct {
+                       __u32 type;
+                       __u16 size;
+                       __u16 reserved;
+               };
+               struct ib_kern_spec_eth     eth;
+               struct ib_kern_spec_ipv4    ipv4;
+               struct ib_kern_spec_tcp_udp tcp_udp;
+       };
+};
+
[Aside note: no IPv6 spec ?]

indeed, but note that the way the specs are defines allow for adding
future spec types w.o breaking anything,
its done in TLV (Type-Length-Value) manner, see the change log of the
IB core main patch  for flow steering
319a441 IB/core: Add receive flow steering support


I've noticed the TLV nature of ib_kern_spec, but a bit too late, so my comments about + 1 and first element being handled differently from the others one are rather pointless.

I will try to provide a patch to propose another way of describing the data (same layout) before v3.12 release.
Hopefully I have more time since Linus hard drive broke ;)

Regards.

--
Yann Droneaud
OPTEYA

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to