Le 17.09.2013 14:35, Matan Barak a écrit :
On 11/9/2013 5:04 PM, Yann Droneaud wrote:
Le mercredi 28 août 2013 à 15:47 +0300, Matan Barak a écrit :
From: Hadar Hen Zion <[email protected]>
Implement ib_uverbs_create_flow and ib_uverbs_destroy_flow to
support flow steering for user space applications.
Change-Id: I0bc6dbe26f4776d00f95b6200ff43ccb24000e31
Signed-off-by: Hadar Hen Zion <[email protected]>
Signed-off-by: Or Gerlitz <[email protected]>
Signed-off-by: Matan Barak <[email protected]>
Seen this in linux-next as commit
436f2ad05a0b65b1467ddf51bc68171c381bf844
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.
This introduces code duplication since we check that value and then
compute it. Since this value has an upper and lower limits and both
boundaries are checked, why is this problematic ?
Cause negative value says: "I don't take care of checking bounds:
Checking bound is a thing for pedantic security researchers
who never wrote real code but write poems in reaction to the non-sense
of life"
Please, check user input first, don't miss the chance to catch error
early.
cmd.flow_attr.size is certainly less then in_len, so you probably mean
cmd.flow_attr.size > (in_len - sizeof(cmd) - sizeof(struct
ib_uverbs_cmd_hdr_ex)).
It's not certainly less than in_len, as the user initializes
cmd.flow_attr.size. Currently, our version of libibverbs place the
entire command size on cmd.flow_attr.size.
I would prefer to push out the fixed known size of the command,
and use this size to hold only the sum of the TLV sizes.
Then you can get more information from the .size field.
+ kern_attr_size < 0 || kern_attr_size >
+ (cmd.flow_attr.num_of_specs * sizeof(struct ib_kern_spec)))
+ return -EINVAL;
+
let's make kern_attr_size unsigned.
I think that it's not a good idea since we're iterating until we've
got to num_of_specs or we've got an invalid size in one of the spec
structures. Since we have an upper limit to kern_attr_size, I don't
think it's problematic to keep it as int.
I think it's not a good idea to iterate until the iterator get negative.
Probably a matter of taste or experience.
If there's an invalid size for TLV, check it before subtract. Not after.
Otherwise it's not bound checking, it's error recovery.
And be stricter: kern_attr_size != cmd.flow_attr.num_of_specs *
sizeof(struct ib_kern_spec) might be welcome.
The structures are TLV, so I can only check upper limit but not the
exact size without traversing them.
OK.
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_flow_attr {
+ __u32 type;
+ __u16 size;
+ __u16 priority;
+ __u8 num_of_specs;
+ __u8 reserved[2];
+ __u8 port;
+ __u32 flags;
+ /* Following are the optional layers according to user request
+ * struct ib_flow_spec_xxx
+ * struct ib_flow_spec_yyy
+ */
+};
+
+struct ib_uverbs_create_flow {
+ __u32 comp_mask;
Alignment notice: There's a hole here on 64bits system with stricter
alignment rules.
Thanks! Do you recommend adding a u32 reserved field ?
Sure it must be added.
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
