Re: switch(4): add more input validations
On Fri, Oct 28, 2016 at 07:56:12PM +0400, Reyk Floeter wrote:
> > On 28.10.2016, at 19:20, Rafael Zalamena wrote:
> > This diff teaches switch(4) how to do more validations on dynamic input
> > field types, like: ofp_match (has N oxms), ofp_action_header (might be
> > followed by N actions) and ofp_instruction (might have N actions inside).
> >
> > This is important because the internal switch structures reuse the ofp_match
> > and friends from the packet and blindly trusts it to be correct, so to be
> > able to do that we must ensure that we are receiving them correctly.
> > We need to pay special attention to the fields that these macros use:
> > - OFP_OXM_FOREACH;
> > - OFP_ACTION_FOREACH;
> > - OFP_FLOW_MOD_MSG_INSTRUCTION_OFFSET;
> > - OFP_FLOW_MOD_INSTRUCTON_FOREACH;
> > - OFP_I_ACTIONS_FOREACH;
> > - OFP_BUCKETS_FOREACH;
> ---snip---
I've commited most of the small diffs, but one of them turned out to be
different so I sent another email ('switch(4): input validation:
swofp_flow_entry_put_instructions').
Here is the diff after all those commits, with just one small change:
I have implemented set_queue action validation that was missing.
ok?
Index: switchofp.c
===
RCS file: /cvs/src/sys/net/switchofp.c,v
retrieving revision 1.23
diff -u -p -r1.23 switchofp.c
--- switchofp.c 31 Oct 2016 08:06:27 - 1.23
+++ switchofp.c 31 Oct 2016 14:52:13 -
@@ -211,8 +211,11 @@ int swofp_flow_cmp_strict(struct swofp_
int swofp_flow_filter(struct swofp_flow_entry *, uint64_t, uint64_t,
uint32_t, uint32_t);
voidswofp_flow_timeout(struct switch_softc *);
+int swofp_validate_oxm(struct ofp_ox_match *, int *);
int swofp_validate_flow_match(struct ofp_match *, int *);
-int swofp_validate_flow_instruction(struct ofp_instruction *, int *);
+int swofp_validate_flow_instruction(struct ofp_instruction *, size_t,
+ int *);
+int swofp_validate_action(struct ofp_action_header *, size_t, int *);
/*
* OpenFlow protocol compare oxm
@@ -1807,75 +1810,242 @@ swofp_ox_cmp_ether_addr(struct ofp_ox_ma
}
}
-
-/* TODO: validation for match */
int
-swofp_validate_flow_match(struct ofp_match *om, int *err)
+swofp_validate_oxm(struct ofp_ox_match *oxm, int *err)
{
- struct ofp_oxm_class *handler;
- struct ofp_ox_match *oxm;
+ struct ofp_oxm_class*handler;
+ int length, hasmask;
+ int neededlen;
- OFP_OXM_FOREACH(om, ntohs(om->om_length), oxm) {
- handler = swofp_lookup_oxm_handler(oxm);
- if (handler == NULL ||
- handler->oxm_match == NULL) {
- *err = OFP_ERRMATCH_BAD_FIELD;
- return (-1);
- }
+ handler = swofp_lookup_oxm_handler(oxm);
+ if (handler == NULL || handler->oxm_match == NULL) {
+ *err = OFP_ERRMATCH_BAD_FIELD;
+ return (-1);
+ }
+
+ hasmask = OFP_OXM_GET_HASMASK(oxm);
+ length = oxm->oxm_length;
+
+ neededlen = (hasmask) ?
+ (handler->oxm_len * 2) : (handler->oxm_len);
+ if (oxm->oxm_length != neededlen) {
+ *err = OFP_ERRMATCH_BAD_LEN;
+ return (-1);
}
return (0);
}
int
-swofp_validate_flow_action_set_field(struct ofp_action_set_field *oasf)
+swofp_validate_flow_match(struct ofp_match *om, int *err)
{
- struct ofp_ox_match *oxm;
- struct ofp_oxm_class*handler;
-
- oxm = (struct ofp_ox_match *)oasf->asf_field;
+ struct ofp_ox_match *oxm;
- handler = swofp_lookup_oxm_handler(oxm);
- if (handler == NULL)
- return (OFP_ERRACTION_SET_TYPE);
- if (handler->oxm_set == NULL)
- return (OFP_ERRACTION_SET_TYPE);
+ /*
+* TODO this function is missing checks for:
+* - OFP_ERRMATCH_BAD_TAG;
+* - OFP_ERRMATCH_BAD_VALUE;
+* - OFP_ERRMATCH_BAD_MASK;
+* - OFP_ERRMATCH_BAD_PREREQ;
+* - OFP_ERRMATCH_DUP_FIELD;
+*/
+ OFP_OXM_FOREACH(om, ntohs(om->om_length), oxm) {
+ if (swofp_validate_oxm(oxm, err))
+ return (*err);
+ }
return (0);
}
-/* TODO: validation for instruction */
int
-swofp_validate_flow_instruction(struct ofp_instruction *oi, int *error)
+swofp_validate_flow_instruction(struct ofp_instruction *oi, size_t total,
+int *err)
{
struct ofp_action_header*oah;
struct ofp_instruction_actions *oia;
+ int ilen;
+
+ ilen = ntohs(oi->i_len);
+ /* Check for bigger than packet or smaller than header. */
+ if (ilen > total || ilen < sizeof(*oi)) {
+ *err = OFP_ERRINST_BAD_LEN;
+ return (-1);
+ }
switch (ntohs(oi->i_type)) {
case OFP_INSTRUCTION_T_GOTO
Re: switch(4): add more input validations
> On 28.10.2016, at 19:20, Rafael Zalamena wrote:
>
> This diff teaches switch(4) how to do more validations on dynamic input
> field types, like: ofp_match (has N oxms), ofp_action_header (might be
> followed by N actions) and ofp_instruction (might have N actions inside).
>
> This is important because the internal switch structures reuse the ofp_match
> and friends from the packet and blindly trusts it to be correct, so to be
> able to do that we must ensure that we are receiving them correctly.
> We need to pay special attention to the fields that these macros use:
> - OFP_OXM_FOREACH;
> - OFP_ACTION_FOREACH;
> - OFP_FLOW_MOD_MSG_INSTRUCTION_OFFSET;
> - OFP_FLOW_MOD_INSTRUCTON_FOREACH;
> - OFP_I_ACTIONS_FOREACH;
> - OFP_BUCKETS_FOREACH;
>
Yes, adding the validation step is the right approach!
And this approach is what we also do in switchd (and iked's ikev2 parser).
> Other small fixes:
Small fixes should go in separately:
> - Simplify OFP_FLOW_MOD_MSG_INSTRUCTION_OFFSET() macro;
OK
> - Change swofp_flow_table_add() malloc behaviour to be like all the rest
> of the code (don't wait for memory);
OK
> - swofp_flow_entry_put_instructions() doesn't need a pointer to an mbuf
> pointer, we only read it;
OK
> - Change the validation function parameters: we can't check for non-errors
> using the value 0, because the spec actually uses it for some errors.
> Instead use int pointer to get error value and use the return value only
> to find out if the validation succeeded or not;
OK
> - Return more accurate error messages: some error messages were being sent
> with the wrong type/code combination;
OK
> - Removed swofp_validate_flow_action_set_field() as it was incorporated
> in action validation;
>
This is part of the validation diff.
Could you commit the small fixes first and send an updated validation diff?
> TODO for next diffs:
> - We still need to code the validation for group messages, but since I
> haven't started using it yet, I didn't touch it;
> - We still need to implement validation for some specific OXM errors:
> duplicated OXM, invalid wildcard, invalid value or missing prereq;
> - swofp_validate_buckets() could use validate_action() with some effort;
>
Sure
> Even though switchd(8) does packet validation (both on input and output),
> we should commit this to enable other vendors/software to use the OpenBSD
> switch(4) directly, otherwise we will need more effort to implement this
> for switchd(8) relaying and force people to use switchd(8) needlessly.
>
Full ack,
a) it is a very important design decision that switch(4) does NOT depend
on switchd(8). This is not 100% true at the moment (as switch(4) doesn't fully
behave like a normal socket, eg. it only supports full single packet read/write
I/O),
but it should be possible to connect any user space controller to /dev/switch0,
eg. by using a tool like socat to connect it to ryu instead of switchd.
b) The kernel should never "trust" user land to do the right thing. Crashing it
from
user input via /dev/switch* would be a bad thing. And we cannot even guarantee
that switchd(8) is absolutely safe.
> ok?
>
Small fixes OK first, see above.
Reyk
> Index: net/ofp.h
> ===
> RCS file: /home/obsdcvs/src/sys/net/ofp.h,v
> retrieving revision 1.2
> diff -u -p -r1.2 ofp.h
> --- net/ofp.h 30 Sep 2016 12:40:00 - 1.2
> +++ net/ofp.h 25 Oct 2016 08:50:14 -
> @@ -95,7 +95,7 @@ struct ofp_hello_element_versionbitmap {
>
> /* Ports */
> #define OFP_PORT_MAX 0xff00 /* Maximum number of physical
> ports */
> -#define OFP_PORT_INPUT 0xfff8 /* Send back to input
> port */
> +#define OFP_PORT_INPUT 0xfff8 /* Send back to input
> port */
> #define OFP_PORT_FLOWTABLE0xfff9 /* Perform actions in flow
> table */
> #define OFP_PORT_NORMAL 0xfffa /* Let switch decide */
> #define OFP_PORT_FLOOD0xfffb /* All non-block ports
> except input */
> @@ -179,9 +179,9 @@ struct ofp_switch_features {
>
> /* Switch capabilities */
> #define OFP_SWCAP_FLOW_STATS 0x1 /* Flow statistics */
> -#define OFP_SWCAP_TABLE_STATS0x2 /* Table statistics */
> -#define OFP_SWCAP_PORT_STATS 0x4 /* Port statistics */
> -#define OFP_SWCAP_GROUP_STATS0x8 /* Group statistics */
> +#define OFP_SWCAP_TABLE_STATS0x2 /* Table statistics */
> +#define OFP_SWCAP_PORT_STATS 0x4 /* Port statistics */
> +#define OFP_SWCAP_GROUP_STATS0x8 /* Group statistics */
> #define OFP_SWCAP_IP_REASM0x20/* Can reassemble IP frags */
> #define OFP_SWCAP_QUEUE_STATS 0x40/* Queue statistics */
> #define OFP_SWCAP_ARP_MATCH_IP0x80/* Match IP addresses
> in ARP pkts */
> @@ -314,15 +314,15 @@ struct ofp_acti
switch(4): add more input validations
This diff teaches switch(4) how to do more validations on dynamic input
field types, like: ofp_match (has N oxms), ofp_action_header (might be
followed by N actions) and ofp_instruction (might have N actions inside).
This is important because the internal switch structures reuse the ofp_match
and friends from the packet and blindly trusts it to be correct, so to be
able to do that we must ensure that we are receiving them correctly.
We need to pay special attention to the fields that these macros use:
- OFP_OXM_FOREACH;
- OFP_ACTION_FOREACH;
- OFP_FLOW_MOD_MSG_INSTRUCTION_OFFSET;
- OFP_FLOW_MOD_INSTRUCTON_FOREACH;
- OFP_I_ACTIONS_FOREACH;
- OFP_BUCKETS_FOREACH;
Other small fixes:
- Simplify OFP_FLOW_MOD_MSG_INSTRUCTION_OFFSET() macro;
- Change swofp_flow_table_add() malloc behaviour to be like all the rest
of the code (don't wait for memory);
- swofp_flow_entry_put_instructions() doesn't need a pointer to an mbuf
pointer, we only read it;
- Change the validation function parameters: we can't check for non-errors
using the value 0, because the spec actually uses it for some errors.
Instead use int pointer to get error value and use the return value only
to find out if the validation succeeded or not;
- Return more accurate error messages: some error messages were being sent
with the wrong type/code combination;
- Removed swofp_validate_flow_action_set_field() as it was incorporated
in action validation;
TODO for next diffs:
- We still need to code the validation for group messages, but since I
haven't started using it yet, I didn't touch it;
- We still need to implement validation for some specific OXM errors:
duplicated OXM, invalid wildcard, invalid value or missing prereq;
- swofp_validate_buckets() could use validate_action() with some effort;
Even though switchd(8) does packet validation (both on input and output),
we should commit this to enable other vendors/software to use the OpenBSD
switch(4) directly, otherwise we will need more effort to implement this
for switchd(8) relaying and force people to use switchd(8) needlessly.
ok?
Index: net/ofp.h
===
RCS file: /home/obsdcvs/src/sys/net/ofp.h,v
retrieving revision 1.2
diff -u -p -r1.2 ofp.h
--- net/ofp.h 30 Sep 2016 12:40:00 - 1.2
+++ net/ofp.h 25 Oct 2016 08:50:14 -
@@ -95,7 +95,7 @@ struct ofp_hello_element_versionbitmap {
/* Ports */
#define OFP_PORT_MAX 0xff00 /* Maximum number of physical
ports */
-#defineOFP_PORT_INPUT 0xfff8 /* Send back to input
port */
+#defineOFP_PORT_INPUT 0xfff8 /* Send back to input
port */
#define OFP_PORT_FLOWTABLE 0xfff9 /* Perform actions in flow
table */
#define OFP_PORT_NORMAL0xfffa /* Let switch decide */
#define OFP_PORT_FLOOD 0xfffb /* All non-block ports except
input */
@@ -179,9 +179,9 @@ struct ofp_switch_features {
/* Switch capabilities */
#define OFP_SWCAP_FLOW_STATS 0x1 /* Flow statistics */
-#define OFP_SWCAP_TABLE_STATS 0x2 /* Table statistics */
-#define OFP_SWCAP_PORT_STATS 0x4 /* Port statistics */
-#define OFP_SWCAP_GROUP_STATS 0x8 /* Group statistics */
+#define OFP_SWCAP_TABLE_STATS 0x2 /* Table statistics */
+#define OFP_SWCAP_PORT_STATS 0x4 /* Port statistics */
+#define OFP_SWCAP_GROUP_STATS 0x8 /* Group statistics */
#define OFP_SWCAP_IP_REASM 0x20/* Can reassemble IP frags */
#define OFP_SWCAP_QUEUE_STATS 0x40/* Queue statistics */
#define OFP_SWCAP_ARP_MATCH_IP 0x80/* Match IP addresses in ARP
pkts */
@@ -314,15 +314,15 @@ struct ofp_action_mpls_ttl {
struct ofp_action_push {
uint16_tap_type;
uint16_tap_len;
- uint16_tap_ethertype;
- uint8_t pad[2];
+ uint16_tap_ethertype;
+ uint8_t ap_pad[2];
} __packed;
struct ofp_action_pop_mpls {
uint16_tapm_type;
uint16_tapm_len;
uint16_tapm_ethertype;
- uint8_t pad[2];
+ uint8_t apm_pad[2];
} __packed;
struct ofp_action_group {
@@ -342,6 +342,12 @@ struct ofp_action_set_field {
uint16_tasf_type;
uint16_tasf_len;
uint8_t asf_field[4];
+} __packed;
+
+struct ofp_action_set_queue {
+ uint16_tasq_type;
+ uint16_tasq_len;
+ uint32_tasq_queue_id;
} __packed;
/* Packet-Out Message */
Index: net/switchofp.c
===
RCS file: /home/obsdcvs/src/sys/net/switchofp.c,v
retrieving revision 1.18
diff -u -p -r1.18 switchofp.c
--- net/switchofp.c 28 Oct 2016 09:01:49 - 1.18
+++ net/switchofp.c 28 Oct 2016 14:48:49
