| Message ID | 20220225171451.16476.7593.stgit@dceara.remote.csb |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix UndefinedBehaviorSanitizer reported issues and enable it in CI. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/intel-ovs-compilation | success | test: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Hi Dumitru, On 2/25/22 18:14, Dumitru Ceara wrote: > Some openflow actions can be misaligned, e.g., actions whithin OF 1.0 > replies to statistics reply messages which have a header of 12 bytes > and no additional padding. > > Also, buggy controllers might incorrectly encode actions. > > When decoding multiple actions in ofpacts_decode(), make sure that > when advancing to the next action it will be properly aligned > (multiple of OFPACT_ALIGNTO). > > Detected by UB Sanitizer when running one of the fuzz tests: > lib/ofp-actions.c:5347:12: runtime error: member access within misaligned address 0x0000016ba274 for type 'const struct nx_action_learn', which requires 8 byte alignment > 0x0000016ba274: note: pointer points here > 20 20 20 20 ff ff 00 38 00 00 23 20 00 10 20 20 20 20 20 20 20 20 20 20 20 20 20 20 00 03 20 00 > ^ > #0 0x52cece in decode_LEARN_common lib/ofp-actions.c:5347 > #1 0x52dcf6 in decode_NXAST_RAW_LEARN lib/ofp-actions.c:5463 > #2 0x548604 in ofpact_decode lib/ofp-actions.inc2:4723 > #3 0x53ee43 in ofpacts_decode lib/ofp-actions.c:7781 > #4 0x53efc1 in ofpacts_pull_openflow_actions__ lib/ofp-actions.c:7820 > #5 0x5409e1 in ofpacts_pull_openflow_instructions lib/ofp-actions.c:8396 > #6 0x5608a8 in ofputil_decode_flow_stats_reply lib/ofp-flow.c:1100 > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > v4: Rebased. > v3: Split out from old patch 07/11. > --- > include/openvswitch/ofp-actions.h | 1 + > include/openvswitch/ofpbuf.h | 2 + > lib/ofp-actions.c | 71 +++++++++++++++++++++++++++++-------- > lib/ofpbuf.c | 39 ++++++++++++++++++++ > 4 files changed, 98 insertions(+), 15 deletions(-) > > diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h > index b7231c7bb334..0991a0177411 100644 > --- a/include/openvswitch/ofp-actions.h > +++ b/include/openvswitch/ofp-actions.h > @@ -203,6 +203,7 @@ BUILD_ASSERT_DECL(sizeof(struct ofpact) == 4); > /* Alignment. */ > #define OFPACT_ALIGNTO 8 > #define OFPACT_ALIGN(SIZE) ROUND_UP(SIZE, OFPACT_ALIGNTO) > +#define OFPACT_IS_ALIGNED(ADDR) ((uintptr_t) (ADDR) % OFPACT_ALIGNTO == 0) > #define OFPACT_PADDED_MEMBERS(MEMBERS) PADDED_MEMBERS(OFPACT_ALIGNTO, MEMBERS) > > /* Returns the ofpact following 'ofpact'. */ > diff --git a/include/openvswitch/ofpbuf.h b/include/openvswitch/ofpbuf.h > index 7b6aba9dc29c..50630c9f9baf 100644 > --- a/include/openvswitch/ofpbuf.h > +++ b/include/openvswitch/ofpbuf.h > @@ -111,6 +111,7 @@ void ofpbuf_use_ds(struct ofpbuf *, const struct ds *); > void ofpbuf_use_stack(struct ofpbuf *, void *, size_t); > void ofpbuf_use_stub(struct ofpbuf *, void *, size_t); > void ofpbuf_use_const(struct ofpbuf *, const void *, size_t); > +void ofpbuf_use_data(struct ofpbuf *, const void *, size_t); > > void ofpbuf_init(struct ofpbuf *, size_t); > void ofpbuf_uninit(struct ofpbuf *); > @@ -149,6 +150,7 @@ static inline size_t ofpbuf_msgsize(const struct ofpbuf *); > void ofpbuf_prealloc_headroom(struct ofpbuf *, size_t); > void ofpbuf_prealloc_tailroom(struct ofpbuf *, size_t); > void ofpbuf_trim(struct ofpbuf *); > +void ofpbuf_align(struct ofpbuf *); > void ofpbuf_padto(struct ofpbuf *, size_t); > void ofpbuf_shift(struct ofpbuf *, int); > > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c > index b24b46d2196b..b95c280eb92e 100644 > --- a/lib/ofp-actions.c > +++ b/lib/ofp-actions.c > @@ -7759,45 +7759,86 @@ check_GOTO_TABLE(const struct ofpact_goto_table *a, > > static void > log_bad_action(const struct ofp_action_header *actions, size_t actions_len, > - const struct ofp_action_header *bad_action, enum ofperr error) > + size_t action_offset, enum ofperr error) > { > if (!VLOG_DROP_WARN(&rl)) { > struct ds s; > > ds_init(&s); > ds_put_hex_dump(&s, actions, actions_len, 0, false); > - VLOG_WARN("bad action at offset %#"PRIxPTR" (%s):\n%s", > - (char *)bad_action - (char *)actions, > + VLOG_WARN("bad action at offset %"PRIuSIZE" (%s):\n%s", action_offset, > ofperr_get_name(error), ds_cstr(&s)); > ds_destroy(&s); > } > } > > static enum ofperr > -ofpacts_decode(const void *actions, size_t actions_len, > - enum ofp_version ofp_version, > - const struct vl_mff_map *vl_mff_map, > - uint64_t *ofpacts_tlv_bitmap, struct ofpbuf *ofpacts) > +ofpacts_decode_aligned(struct ofpbuf *openflow, enum ofp_version ofp_version, > + const struct vl_mff_map *vl_mff_map, > + uint64_t *ofpacts_tlv_bitmap, struct ofpbuf *ofpacts, > + size_t *bad_action_offset) > { > - struct ofpbuf openflow = ofpbuf_const_initializer(actions, actions_len); > - while (openflow.size) { > - const struct ofp_action_header *action = openflow.data; > + size_t decoded_len = 0; > + enum ofperr error = 0; > + > + ovs_assert(OFPACT_IS_ALIGNED(openflow->data)); > + while (openflow->size) { > + /* Ensure the next action data is properly aligned before decoding it. > + * Some times it's valid to have to decode actions that are not > + * properly aligned (e.g., when processing OF 1.0 statistics reply > + * messages which have a header of 12 bytes - struct ofp10_stats_msg). > + * In other cases the encoder might be buggy. > + */ > + if (!OFPACT_IS_ALIGNED(openflow->data)) { > + ofpbuf_align(openflow); > + } > + > + const struct ofp_action_header *action = openflow->data; > enum ofp_raw_action_type raw; > - enum ofperr error; > uint64_t arg; > > - error = ofpact_pull_raw(&openflow, ofp_version, &raw, &arg); > + error = ofpact_pull_raw(openflow, ofp_version, &raw, &arg); > if (!error) { > error = ofpact_decode(action, raw, ofp_version, arg, vl_mff_map, > ofpacts_tlv_bitmap, ofpacts); > } > > if (error) { > - log_bad_action(actions, actions_len, action, error); > - return error; > + *bad_action_offset = decoded_len; > + goto done; > } > + decoded_len += openflow->size; Doesn't "openflow->size" start being equal to the size of the entire action buffer? If I am reading this code right, shouldn't it be "decoded_len += action->len"? > } > - return 0; > + > +done: > + return error; > +} > + > +static enum ofperr > +ofpacts_decode(const void *actions, size_t actions_len, > + enum ofp_version ofp_version, > + const struct vl_mff_map *vl_mff_map, > + uint64_t *ofpacts_tlv_bitmap, struct ofpbuf *ofpacts) > +{ > + size_t bad_action_offset = 0; > + struct ofpbuf aligned_buf; > + > + if (!OFPACT_IS_ALIGNED(actions)) { > + ofpbuf_init(&aligned_buf, actions_len); > + ofpbuf_put(&aligned_buf, actions, actions_len); > + } else { > + ofpbuf_use_data(&aligned_buf, actions, actions_len); > + } > + > + enum ofperr error > + = ofpacts_decode_aligned(&aligned_buf, ofp_version, vl_mff_map, > + ofpacts_tlv_bitmap, ofpacts, > + &bad_action_offset); > + if (error) { > + log_bad_action(actions, actions_len, bad_action_offset, error); > + } > + ofpbuf_uninit(&aligned_buf); > + return error; > } > > static enum ofperr > diff --git a/lib/ofpbuf.c b/lib/ofpbuf.c > index 0330681b2518..b79d89e71447 100644 > --- a/lib/ofpbuf.c > +++ b/lib/ofpbuf.c > @@ -115,6 +115,24 @@ ofpbuf_use_const(struct ofpbuf *b, const void *data, size_t size) > ofpbuf_use__(b, CONST_CAST(void *, data), size, size, OFPBUF_STACK); > } > > +/* Initializes 'b' as an ofpbuf whose data starts at 'data' and continues for > + * 'size' bytes. This is appropriate for an ofpbuf that will be mostly used to > + * inspect existing data, however, if needed, data may be occasionally changed. > + * > + * The first time the data is changed the provided buffer will be copied into > + * a malloc()'d buffer. Thus, it is wise to call ofpbuf_uninit() on an ofpbuf > + * initialized by this function, so that if it expanded into the heap, that > + * memory is freed. > + * > + * 'base' should be appropriately aligned. Using an array of uint32_t or > + * uint64_t for the buffer is a reasonable way to ensure appropriate alignment > + * for 32- or 64-bit data. */ In this case, I think the comment should refer to 'data' instead of 'base'. > +void > +ofpbuf_use_data(struct ofpbuf *b, const void *data, size_t size) > +{ > + ofpbuf_use__(b, CONST_CAST(void *, data), size, size, OFPBUF_STUB); > +} > + > /* Initializes 'b' as an empty ofpbuf with an initial capacity of 'size' > * bytes. */ > void > @@ -322,6 +340,27 @@ ofpbuf_trim(struct ofpbuf *b) > } > } > > +/* Re-aligns the buffer data. Relies on malloc() to ensure proper alignment. > + * > + * This function should not be called for buffers of type OFPBUF_STUB. > + */ s/OFPBUF_STUB/OFPBUF_STACK/? > +void > +ofpbuf_align(struct ofpbuf *b) > +{ > + switch (b->source) { > + case OFPBUF_MALLOC: > + case OFPBUF_STUB: > + /* Resizing 'b' always reallocates the buffer, ensuring proper > + * alignment. > + */ > + ofpbuf_resize__(b, 0, 0); > + break; > + case OFPBUF_STACK: > + OVS_NOT_REACHED(); > + break; > + } > +} > + > /* If 'b' is shorter than 'length' bytes, pads its tail out with zeros to that > * length. */ > void >
On 3/1/22 09:30, Adrian Moreno wrote: > Hi Dumitru, > Hi Adrian, Thanks for the review! > On 2/25/22 18:14, Dumitru Ceara wrote: >> Some openflow actions can be misaligned, e.g., actions whithin OF 1.0 >> replies to statistics reply messages which have a header of 12 bytes >> and no additional padding. >> >> Also, buggy controllers might incorrectly encode actions. >> >> When decoding multiple actions in ofpacts_decode(), make sure that >> when advancing to the next action it will be properly aligned >> (multiple of OFPACT_ALIGNTO). >> >> Detected by UB Sanitizer when running one of the fuzz tests: >> lib/ofp-actions.c:5347:12: runtime error: member access within >> misaligned address 0x0000016ba274 for type 'const struct >> nx_action_learn', which requires 8 byte alignment >> 0x0000016ba274: note: pointer points here >> 20 20 20 20 ff ff 00 38 00 00 23 20 00 10 20 20 20 20 20 20 20 >> 20 20 20 20 20 20 20 00 03 20 00 >> ^ >> #0 0x52cece in decode_LEARN_common lib/ofp-actions.c:5347 >> #1 0x52dcf6 in decode_NXAST_RAW_LEARN lib/ofp-actions.c:5463 >> #2 0x548604 in ofpact_decode lib/ofp-actions.inc2:4723 >> #3 0x53ee43 in ofpacts_decode lib/ofp-actions.c:7781 >> #4 0x53efc1 in ofpacts_pull_openflow_actions__ >> lib/ofp-actions.c:7820 >> #5 0x5409e1 in ofpacts_pull_openflow_instructions >> lib/ofp-actions.c:8396 >> #6 0x5608a8 in ofputil_decode_flow_stats_reply lib/ofp-flow.c:1100 >> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> v4: Rebased. >> v3: Split out from old patch 07/11. >> --- >> include/openvswitch/ofp-actions.h | 1 + >> include/openvswitch/ofpbuf.h | 2 + >> lib/ofp-actions.c | 71 >> +++++++++++++++++++++++++++++-------- >> lib/ofpbuf.c | 39 ++++++++++++++++++++ >> 4 files changed, 98 insertions(+), 15 deletions(-) >> >> diff --git a/include/openvswitch/ofp-actions.h >> b/include/openvswitch/ofp-actions.h >> index b7231c7bb334..0991a0177411 100644 >> --- a/include/openvswitch/ofp-actions.h >> +++ b/include/openvswitch/ofp-actions.h >> @@ -203,6 +203,7 @@ BUILD_ASSERT_DECL(sizeof(struct ofpact) == 4); >> /* Alignment. */ >> #define OFPACT_ALIGNTO 8 >> #define OFPACT_ALIGN(SIZE) ROUND_UP(SIZE, OFPACT_ALIGNTO) >> +#define OFPACT_IS_ALIGNED(ADDR) ((uintptr_t) (ADDR) % OFPACT_ALIGNTO >> == 0) >> #define OFPACT_PADDED_MEMBERS(MEMBERS) >> PADDED_MEMBERS(OFPACT_ALIGNTO, MEMBERS) >> /* Returns the ofpact following 'ofpact'. */ >> diff --git a/include/openvswitch/ofpbuf.h b/include/openvswitch/ofpbuf.h >> index 7b6aba9dc29c..50630c9f9baf 100644 >> --- a/include/openvswitch/ofpbuf.h >> +++ b/include/openvswitch/ofpbuf.h >> @@ -111,6 +111,7 @@ void ofpbuf_use_ds(struct ofpbuf *, const struct >> ds *); >> void ofpbuf_use_stack(struct ofpbuf *, void *, size_t); >> void ofpbuf_use_stub(struct ofpbuf *, void *, size_t); >> void ofpbuf_use_const(struct ofpbuf *, const void *, size_t); >> +void ofpbuf_use_data(struct ofpbuf *, const void *, size_t); >> void ofpbuf_init(struct ofpbuf *, size_t); >> void ofpbuf_uninit(struct ofpbuf *); >> @@ -149,6 +150,7 @@ static inline size_t ofpbuf_msgsize(const struct >> ofpbuf *); >> void ofpbuf_prealloc_headroom(struct ofpbuf *, size_t); >> void ofpbuf_prealloc_tailroom(struct ofpbuf *, size_t); >> void ofpbuf_trim(struct ofpbuf *); >> +void ofpbuf_align(struct ofpbuf *); >> void ofpbuf_padto(struct ofpbuf *, size_t); >> void ofpbuf_shift(struct ofpbuf *, int); >> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c >> index b24b46d2196b..b95c280eb92e 100644 >> --- a/lib/ofp-actions.c >> +++ b/lib/ofp-actions.c >> @@ -7759,45 +7759,86 @@ check_GOTO_TABLE(const struct >> ofpact_goto_table *a, >> >> static void >> log_bad_action(const struct ofp_action_header *actions, size_t >> actions_len, >> - const struct ofp_action_header *bad_action, enum >> ofperr error) >> + size_t action_offset, enum ofperr error) >> { >> if (!VLOG_DROP_WARN(&rl)) { >> struct ds s; >> ds_init(&s); >> ds_put_hex_dump(&s, actions, actions_len, 0, false); >> - VLOG_WARN("bad action at offset %#"PRIxPTR" (%s):\n%s", >> - (char *)bad_action - (char *)actions, >> + VLOG_WARN("bad action at offset %"PRIuSIZE" (%s):\n%s", >> action_offset, >> ofperr_get_name(error), ds_cstr(&s)); >> ds_destroy(&s); >> } >> } >> static enum ofperr >> -ofpacts_decode(const void *actions, size_t actions_len, >> - enum ofp_version ofp_version, >> - const struct vl_mff_map *vl_mff_map, >> - uint64_t *ofpacts_tlv_bitmap, struct ofpbuf *ofpacts) >> +ofpacts_decode_aligned(struct ofpbuf *openflow, enum ofp_version >> ofp_version, >> + const struct vl_mff_map *vl_mff_map, >> + uint64_t *ofpacts_tlv_bitmap, struct ofpbuf >> *ofpacts, >> + size_t *bad_action_offset) >> { >> - struct ofpbuf openflow = ofpbuf_const_initializer(actions, >> actions_len); >> - while (openflow.size) { >> - const struct ofp_action_header *action = openflow.data; >> + size_t decoded_len = 0; >> + enum ofperr error = 0; >> + >> + ovs_assert(OFPACT_IS_ALIGNED(openflow->data)); >> + while (openflow->size) { >> + /* Ensure the next action data is properly aligned before >> decoding it. >> + * Some times it's valid to have to decode actions that are not >> + * properly aligned (e.g., when processing OF 1.0 statistics >> reply >> + * messages which have a header of 12 bytes - struct >> ofp10_stats_msg). >> + * In other cases the encoder might be buggy. >> + */ >> + if (!OFPACT_IS_ALIGNED(openflow->data)) { >> + ofpbuf_align(openflow); >> + } >> + >> + const struct ofp_action_header *action = openflow->data; >> enum ofp_raw_action_type raw; >> - enum ofperr error; >> uint64_t arg; >> - error = ofpact_pull_raw(&openflow, ofp_version, &raw, &arg); >> + error = ofpact_pull_raw(openflow, ofp_version, &raw, &arg); >> if (!error) { >> error = ofpact_decode(action, raw, ofp_version, arg, >> vl_mff_map, >> ofpacts_tlv_bitmap, ofpacts); >> } >> if (error) { >> - log_bad_action(actions, actions_len, action, error); >> - return error; >> + *bad_action_offset = decoded_len; >> + goto done; >> } >> + decoded_len += openflow->size; > > Doesn't "openflow->size" start being equal to the size of the entire > action buffer? > If I am reading this code right, shouldn't it be "decoded_len += > action->len"? > Good catch! You're right, this should be 'action->len'. >> } >> - return 0; >> + >> +done: >> + return error; >> +} >> + >> +static enum ofperr >> +ofpacts_decode(const void *actions, size_t actions_len, >> + enum ofp_version ofp_version, >> + const struct vl_mff_map *vl_mff_map, >> + uint64_t *ofpacts_tlv_bitmap, struct ofpbuf *ofpacts) >> +{ >> + size_t bad_action_offset = 0; >> + struct ofpbuf aligned_buf; >> + >> + if (!OFPACT_IS_ALIGNED(actions)) { >> + ofpbuf_init(&aligned_buf, actions_len); >> + ofpbuf_put(&aligned_buf, actions, actions_len); >> + } else { >> + ofpbuf_use_data(&aligned_buf, actions, actions_len); >> + } >> + >> + enum ofperr error >> + = ofpacts_decode_aligned(&aligned_buf, ofp_version, vl_mff_map, >> + ofpacts_tlv_bitmap, ofpacts, >> + &bad_action_offset); >> + if (error) { >> + log_bad_action(actions, actions_len, bad_action_offset, error); >> + } >> + ofpbuf_uninit(&aligned_buf); >> + return error; >> } >> static enum ofperr >> diff --git a/lib/ofpbuf.c b/lib/ofpbuf.c >> index 0330681b2518..b79d89e71447 100644 >> --- a/lib/ofpbuf.c >> +++ b/lib/ofpbuf.c >> @@ -115,6 +115,24 @@ ofpbuf_use_const(struct ofpbuf *b, const void >> *data, size_t size) >> ofpbuf_use__(b, CONST_CAST(void *, data), size, size, >> OFPBUF_STACK); >> } >> +/* Initializes 'b' as an ofpbuf whose data starts at 'data' and >> continues for >> + * 'size' bytes. This is appropriate for an ofpbuf that will be >> mostly used to >> + * inspect existing data, however, if needed, data may be >> occasionally changed. >> + * >> + * The first time the data is changed the provided buffer will be >> copied into >> + * a malloc()'d buffer. Thus, it is wise to call ofpbuf_uninit() on >> an ofpbuf >> + * initialized by this function, so that if it expanded into the >> heap, that >> + * memory is freed. >> + * >> + * 'base' should be appropriately aligned. Using an array of >> uint32_t or >> + * uint64_t for the buffer is a reasonable way to ensure appropriate >> alignment >> + * for 32- or 64-bit data. */ > > In this case, I think the comment should refer to 'data' instead of 'base'. > Right, my bad. >> +void >> +ofpbuf_use_data(struct ofpbuf *b, const void *data, size_t size) >> +{ >> + ofpbuf_use__(b, CONST_CAST(void *, data), size, size, OFPBUF_STUB); >> +} >> + >> /* Initializes 'b' as an empty ofpbuf with an initial capacity of >> 'size' >> * bytes. */ >> void >> @@ -322,6 +340,27 @@ ofpbuf_trim(struct ofpbuf *b) >> } >> } >> +/* Re-aligns the buffer data. Relies on malloc() to ensure proper >> alignment. >> + * >> + * This function should not be called for buffers of type OFPBUF_STUB. >> + */ > > s/OFPBUF_STUB/OFPBUF_STACK/? > Ack. >> +void >> +ofpbuf_align(struct ofpbuf *b) >> +{ >> + switch (b->source) { >> + case OFPBUF_MALLOC: >> + case OFPBUF_STUB: >> + /* Resizing 'b' always reallocates the buffer, ensuring proper >> + * alignment. >> + */ >> + ofpbuf_resize__(b, 0, 0); >> + break; >> + case OFPBUF_STACK: >> + OVS_NOT_REACHED(); >> + break; >> + } >> +} >> + >> /* If 'b' is shorter than 'length' bytes, pads its tail out with >> zeros to that >> * length. */ >> void >> > Thanks, Dumitru
diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h index b7231c7bb334..0991a0177411 100644 --- a/include/openvswitch/ofp-actions.h +++ b/include/openvswitch/ofp-actions.h @@ -203,6 +203,7 @@ BUILD_ASSERT_DECL(sizeof(struct ofpact) == 4); /* Alignment. */ #define OFPACT_ALIGNTO 8 #define OFPACT_ALIGN(SIZE) ROUND_UP(SIZE, OFPACT_ALIGNTO) +#define OFPACT_IS_ALIGNED(ADDR) ((uintptr_t) (ADDR) % OFPACT_ALIGNTO == 0) #define OFPACT_PADDED_MEMBERS(MEMBERS) PADDED_MEMBERS(OFPACT_ALIGNTO, MEMBERS) /* Returns the ofpact following 'ofpact'. */ diff --git a/include/openvswitch/ofpbuf.h b/include/openvswitch/ofpbuf.h index 7b6aba9dc29c..50630c9f9baf 100644 --- a/include/openvswitch/ofpbuf.h +++ b/include/openvswitch/ofpbuf.h @@ -111,6 +111,7 @@ void ofpbuf_use_ds(struct ofpbuf *, const struct ds *); void ofpbuf_use_stack(struct ofpbuf *, void *, size_t); void ofpbuf_use_stub(struct ofpbuf *, void *, size_t); void ofpbuf_use_const(struct ofpbuf *, const void *, size_t); +void ofpbuf_use_data(struct ofpbuf *, const void *, size_t); void ofpbuf_init(struct ofpbuf *, size_t); void ofpbuf_uninit(struct ofpbuf *); @@ -149,6 +150,7 @@ static inline size_t ofpbuf_msgsize(const struct ofpbuf *); void ofpbuf_prealloc_headroom(struct ofpbuf *, size_t); void ofpbuf_prealloc_tailroom(struct ofpbuf *, size_t); void ofpbuf_trim(struct ofpbuf *); +void ofpbuf_align(struct ofpbuf *); void ofpbuf_padto(struct ofpbuf *, size_t); void ofpbuf_shift(struct ofpbuf *, int); diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index b24b46d2196b..b95c280eb92e 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -7759,45 +7759,86 @@ check_GOTO_TABLE(const struct ofpact_goto_table *a, static void log_bad_action(const struct ofp_action_header *actions, size_t actions_len, - const struct ofp_action_header *bad_action, enum ofperr error) + size_t action_offset, enum ofperr error) { if (!VLOG_DROP_WARN(&rl)) { struct ds s; ds_init(&s); ds_put_hex_dump(&s, actions, actions_len, 0, false); - VLOG_WARN("bad action at offset %#"PRIxPTR" (%s):\n%s", - (char *)bad_action - (char *)actions, + VLOG_WARN("bad action at offset %"PRIuSIZE" (%s):\n%s", action_offset, ofperr_get_name(error), ds_cstr(&s)); ds_destroy(&s); } } static enum ofperr -ofpacts_decode(const void *actions, size_t actions_len, - enum ofp_version ofp_version, - const struct vl_mff_map *vl_mff_map, - uint64_t *ofpacts_tlv_bitmap, struct ofpbuf *ofpacts) +ofpacts_decode_aligned(struct ofpbuf *openflow, enum ofp_version ofp_version, + const struct vl_mff_map *vl_mff_map, + uint64_t *ofpacts_tlv_bitmap, struct ofpbuf *ofpacts, + size_t *bad_action_offset) { - struct ofpbuf openflow = ofpbuf_const_initializer(actions, actions_len); - while (openflow.size) { - const struct ofp_action_header *action = openflow.data; + size_t decoded_len = 0; + enum ofperr error = 0; + + ovs_assert(OFPACT_IS_ALIGNED(openflow->data)); + while (openflow->size) { + /* Ensure the next action data is properly aligned before decoding it. + * Some times it's valid to have to decode actions that are not + * properly aligned (e.g., when processing OF 1.0 statistics reply + * messages which have a header of 12 bytes - struct ofp10_stats_msg). + * In other cases the encoder might be buggy. + */ + if (!OFPACT_IS_ALIGNED(openflow->data)) { + ofpbuf_align(openflow); + } + + const struct ofp_action_header *action = openflow->data; enum ofp_raw_action_type raw; - enum ofperr error; uint64_t arg; - error = ofpact_pull_raw(&openflow, ofp_version, &raw, &arg); + error = ofpact_pull_raw(openflow, ofp_version, &raw, &arg); if (!error) { error = ofpact_decode(action, raw, ofp_version, arg, vl_mff_map, ofpacts_tlv_bitmap, ofpacts); } if (error) { - log_bad_action(actions, actions_len, action, error); - return error; + *bad_action_offset = decoded_len; + goto done; } + decoded_len += openflow->size; } - return 0; + +done: + return error; +} + +static enum ofperr +ofpacts_decode(const void *actions, size_t actions_len, + enum ofp_version ofp_version, + const struct vl_mff_map *vl_mff_map, + uint64_t *ofpacts_tlv_bitmap, struct ofpbuf *ofpacts) +{ + size_t bad_action_offset = 0; + struct ofpbuf aligned_buf; + + if (!OFPACT_IS_ALIGNED(actions)) { + ofpbuf_init(&aligned_buf, actions_len); + ofpbuf_put(&aligned_buf, actions, actions_len); + } else { + ofpbuf_use_data(&aligned_buf, actions, actions_len); + } + + enum ofperr error + = ofpacts_decode_aligned(&aligned_buf, ofp_version, vl_mff_map, + ofpacts_tlv_bitmap, ofpacts, + &bad_action_offset); + if (error) { + log_bad_action(actions, actions_len, bad_action_offset, error); + } + ofpbuf_uninit(&aligned_buf); + return error; } static enum ofperr diff --git a/lib/ofpbuf.c b/lib/ofpbuf.c index 0330681b2518..b79d89e71447 100644 --- a/lib/ofpbuf.c +++ b/lib/ofpbuf.c @@ -115,6 +115,24 @@ ofpbuf_use_const(struct ofpbuf *b, const void *data, size_t size) ofpbuf_use__(b, CONST_CAST(void *, data), size, size, OFPBUF_STACK); } +/* Initializes 'b' as an ofpbuf whose data starts at 'data' and continues for + * 'size' bytes. This is appropriate for an ofpbuf that will be mostly used to + * inspect existing data, however, if needed, data may be occasionally changed. + * + * The first time the data is changed the provided buffer will be copied into + * a malloc()'d buffer. Thus, it is wise to call ofpbuf_uninit() on an ofpbuf + * initialized by this function, so that if it expanded into the heap, that + * memory is freed. + * + * 'base' should be appropriately aligned. Using an array of uint32_t or + * uint64_t for the buffer is a reasonable way to ensure appropriate alignment + * for 32- or 64-bit data. */ +void +ofpbuf_use_data(struct ofpbuf *b, const void *data, size_t size) +{ + ofpbuf_use__(b, CONST_CAST(void *, data), size, size, OFPBUF_STUB); +} + /* Initializes 'b' as an empty ofpbuf with an initial capacity of 'size' * bytes. */ void @@ -322,6 +340,27 @@ ofpbuf_trim(struct ofpbuf *b) } } +/* Re-aligns the buffer data. Relies on malloc() to ensure proper alignment. + * + * This function should not be called for buffers of type OFPBUF_STUB. + */ +void +ofpbuf_align(struct ofpbuf *b) +{ + switch (b->source) { + case OFPBUF_MALLOC: + case OFPBUF_STUB: + /* Resizing 'b' always reallocates the buffer, ensuring proper + * alignment. + */ + ofpbuf_resize__(b, 0, 0); + break; + case OFPBUF_STACK: + OVS_NOT_REACHED(); + break; + } +} + /* If 'b' is shorter than 'length' bytes, pads its tail out with zeros to that * length. */ void
Some openflow actions can be misaligned, e.g., actions whithin OF 1.0 replies to statistics reply messages which have a header of 12 bytes and no additional padding. Also, buggy controllers might incorrectly encode actions. When decoding multiple actions in ofpacts_decode(), make sure that when advancing to the next action it will be properly aligned (multiple of OFPACT_ALIGNTO). Detected by UB Sanitizer when running one of the fuzz tests: lib/ofp-actions.c:5347:12: runtime error: member access within misaligned address 0x0000016ba274 for type 'const struct nx_action_learn', which requires 8 byte alignment 0x0000016ba274: note: pointer points here 20 20 20 20 ff ff 00 38 00 00 23 20 00 10 20 20 20 20 20 20 20 20 20 20 20 20 20 20 00 03 20 00 ^ #0 0x52cece in decode_LEARN_common lib/ofp-actions.c:5347 #1 0x52dcf6 in decode_NXAST_RAW_LEARN lib/ofp-actions.c:5463 #2 0x548604 in ofpact_decode lib/ofp-actions.inc2:4723 #3 0x53ee43 in ofpacts_decode lib/ofp-actions.c:7781 #4 0x53efc1 in ofpacts_pull_openflow_actions__ lib/ofp-actions.c:7820 #5 0x5409e1 in ofpacts_pull_openflow_instructions lib/ofp-actions.c:8396 #6 0x5608a8 in ofputil_decode_flow_stats_reply lib/ofp-flow.c:1100 Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- v4: Rebased. v3: Split out from old patch 07/11. --- include/openvswitch/ofp-actions.h | 1 + include/openvswitch/ofpbuf.h | 2 + lib/ofp-actions.c | 71 +++++++++++++++++++++++++++++-------- lib/ofpbuf.c | 39 ++++++++++++++++++++ 4 files changed, 98 insertions(+), 15 deletions(-)