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 <[email protected]> >> --- >> 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
