| Message ID | 20211221110259.14345.22691.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/github-robot-_Build_and_Test | fail | github build: failed |
On 12/21/21 12:03, Dumitru Ceara wrote: > When decoding multiple actions in ofpacts_decode(), make sure that when > we advance to the next action it will be properly aligned (multiple of > OFPACT_ALIGNTO). If not, clone or trim the ofpbuf to ensure proper > aligment. If some of the ofpact entries are not aligned, it mean that there is a bug in encoder. All actions must be aligned. Working around that on the decoder's side doesn't seem like a good solution. Or am I missing something? Best regards, Ilya Maximets. > > One example is parsing the OVN "eth.dst[40] = 1;" action, which > triggered the following warning from UndefinedBehaviorSanitizer: > > lib/meta-flow.c:3210:9: runtime error: member access within misaligned address 0x000000de4e36 for type 'const union mf_value', which requires 8 byte alignment > 0x000000de4e36: note: pointer points here > 00 00 00 00 01 00 00 00 00 00 00 00 00 00 70 4e de 00 00 00 00 00 10 51 de 00 00 00 00 00 c0 4f > ^ > #0 0x5818bc in mf_format lib/meta-flow.c:3210 > #1 0x5b6047 in format_SET_FIELD lib/ofp-actions.c:3342 > #2 0x5d68ab in ofpact_format lib/ofp-actions.c:9213 > #3 0x5d6ee0 in ofpacts_format lib/ofp-actions.c:9237 > #4 0x410922 in test_parse_actions tests/test-ovn.c:1360 > > Another example is 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 > [...] > > Or: > lib/ofp-print.c:1218:24: runtime error: member access within misaligned address 0x0000019229d2 for type 'const struct ofp_header', which requires 4 byte alignment > 0x0000019229d2: note: pointer points here > 00 00 5a 5a 05 22 00 3e 00 00 00 09 00 00 00 00 00 00 00 03 05 0d 00 2e 00 00 00 09 ff ff ff ff > ^ > #0 0x7d45cc in ofp_to_string lib/ofp-print.c:1218 > #1 0x774fa8 in ofperr_msg_format lib/ofp-errors.c:253 > #2 0x7d2617 in ofp_print_error_msg lib/ofp-print.c:435 > #3 0x7d3eb7 in ofp_to_string__ lib/ofp-print.c:998 > #4 0x7d47fb in ofp_to_string lib/ofp-print.c:1244 > #5 0x8dcc4b in do_send lib/vconn.c:688 > #6 0x8dca64 in vconn_send lib/vconn.c:671 > [...] > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > include/openvswitch/ofp-actions.h | 1 + > include/openvswitch/util.h | 3 +++ > lib/meta-flow.c | 6 +++++ > lib/nx-match.c | 7 ++++++ > lib/ofp-actions.c | 44 +++++++++++++++++++++++++++---------- > lib/ofp-errors.c | 2 ++ > ofproto/ofproto-dpif-xlate.c | 13 ++++++++--- > 7 files changed, 60 insertions(+), 16 deletions(-)
On 1/4/22 13:15, Ilya Maximets wrote: > On 12/21/21 12:03, Dumitru Ceara wrote: >> When decoding multiple actions in ofpacts_decode(), make sure that when >> we advance to the next action it will be properly aligned (multiple of >> OFPACT_ALIGNTO). If not, clone or trim the ofpbuf to ensure proper >> aligment. > > If some of the ofpact entries are not aligned, it mean that there is > a bug in encoder. All actions must be aligned. Working around that > on the decoder's side doesn't seem like a good solution. Or am I > missing something? There are 3 problems and maybe I should've split this patch in smaller changes: 1. set-field-with-mask action: The way the ofpact_set_field is encoded, the mask (of type mf_value) will start at a misaligned offset for ethernet address fields. We could try to change the encoder to add padding between the "value" and the "mask" but I guess that's not acceptable as this is a user visible interface, right? The way I wanted to fix this is by first checking if the mask field is aligned before accessing it (IS_PTR_ALIGNED). I'm open to alternatives though. 2. ofperr_decode_msg() would fail to align the error payload if b.size was not aligned, potentially returning an ofpbuf with data aligned at 2 bytes. 3. The message is incorrectly encoded. In our case, one of the fuzz regression tests happens to do this but there's nothing preventing a buggy controller from doing this in real life AFAICT. I don't think we can do more than just check that the ofpact is aligned before reading it. With the current patch, I'm cloning the action starting at the misaligned address but based on your comment, I guess we can just return an error when the decoder detects a misaligned ofpact. > > Best regards, Ilya Maximets. > Thanks, Dumitru >> >> One example is parsing the OVN "eth.dst[40] = 1;" action, which >> triggered the following warning from UndefinedBehaviorSanitizer: >> >> lib/meta-flow.c:3210:9: runtime error: member access within misaligned address 0x000000de4e36 for type 'const union mf_value', which requires 8 byte alignment >> 0x000000de4e36: note: pointer points here >> 00 00 00 00 01 00 00 00 00 00 00 00 00 00 70 4e de 00 00 00 00 00 10 51 de 00 00 00 00 00 c0 4f >> ^ >> #0 0x5818bc in mf_format lib/meta-flow.c:3210 >> #1 0x5b6047 in format_SET_FIELD lib/ofp-actions.c:3342 >> #2 0x5d68ab in ofpact_format lib/ofp-actions.c:9213 >> #3 0x5d6ee0 in ofpacts_format lib/ofp-actions.c:9237 >> #4 0x410922 in test_parse_actions tests/test-ovn.c:1360 >> >> Another example is 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 >> [...] >> >> Or: >> lib/ofp-print.c:1218:24: runtime error: member access within misaligned address 0x0000019229d2 for type 'const struct ofp_header', which requires 4 byte alignment >> 0x0000019229d2: note: pointer points here >> 00 00 5a 5a 05 22 00 3e 00 00 00 09 00 00 00 00 00 00 00 03 05 0d 00 2e 00 00 00 09 ff ff ff ff >> ^ >> #0 0x7d45cc in ofp_to_string lib/ofp-print.c:1218 >> #1 0x774fa8 in ofperr_msg_format lib/ofp-errors.c:253 >> #2 0x7d2617 in ofp_print_error_msg lib/ofp-print.c:435 >> #3 0x7d3eb7 in ofp_to_string__ lib/ofp-print.c:998 >> #4 0x7d47fb in ofp_to_string lib/ofp-print.c:1244 >> #5 0x8dcc4b in do_send lib/vconn.c:688 >> #6 0x8dca64 in vconn_send lib/vconn.c:671 >> [...] >> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> include/openvswitch/ofp-actions.h | 1 + >> include/openvswitch/util.h | 3 +++ >> lib/meta-flow.c | 6 +++++ >> lib/nx-match.c | 7 ++++++ >> lib/ofp-actions.c | 44 +++++++++++++++++++++++++++---------- >> lib/ofp-errors.c | 2 ++ >> ofproto/ofproto-dpif-xlate.c | 13 ++++++++--- >> 7 files changed, 60 insertions(+), 16 deletions(-) > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h index 41bcb55d2056..3b67675fe02a 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/util.h b/include/openvswitch/util.h index cfbd5f1a3375..9a22ed56c505 100644 --- a/include/openvswitch/util.h +++ b/include/openvswitch/util.h @@ -285,6 +285,9 @@ is_pow2(uintmax_t x) * segfault, so it is important to be aware of correct alignment. */ #define ALIGNED_CAST(TYPE, ATTR) ((TYPE) (void *) (ATTR)) +#define IS_PTR_ALIGNED(OBJ) \ + (!(OBJ) || (uintptr_t) (OBJ) % __alignof__(OVS_TYPEOF(OBJ)) == 0) + #ifdef __cplusplus } #endif diff --git a/lib/meta-flow.c b/lib/meta-flow.c index e03cd8d0c5cd..92bfd350a92b 100644 --- a/lib/meta-flow.c +++ b/lib/meta-flow.c @@ -3172,6 +3172,8 @@ mf_format(const struct mf_field *mf, const struct ofputil_port_map *port_map, struct ds *s) { + union mf_value aligned_mask; + if (mask) { if (is_all_zeros(mask, mf->n_bytes)) { ds_put_cstr(s, "ANY"); @@ -3207,6 +3209,10 @@ mf_format(const struct mf_field *mf, break; case MFS_ETHERNET: + if (!IS_PTR_ALIGNED(mask)) { + memcpy(&aligned_mask.mac, mask, sizeof aligned_mask.mac); + mask = &aligned_mask; + } eth_format_masked(value->mac, mask ? &mask->mac : NULL, s); break; diff --git a/lib/nx-match.c b/lib/nx-match.c index 440f5f7630c9..6e87deffc0d0 100644 --- a/lib/nx-match.c +++ b/lib/nx-match.c @@ -31,6 +31,7 @@ #include "openvswitch/ofp-match.h" #include "openvswitch/ofp-port.h" #include "openvswitch/ofpbuf.h" +#include "openvswitch/util.h" #include "openvswitch/vlog.h" #include "packets.h" #include "openvswitch/shash.h" @@ -1491,9 +1492,15 @@ nx_put_entry(struct ofpbuf *b, const struct mf_field *mff, enum ofp_version version, const union mf_value *value, const union mf_value *mask) { + union mf_value aligned_mask; bool masked; int len, offset; + if (!IS_PTR_ALIGNED(mask)) { + memcpy(&aligned_mask, mask, mff->n_bytes); + mask = &aligned_mask; + } + len = mf_field_len(mff, value, mask, &masked); offset = mff->n_bytes - len; diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index f21b3c579f2b..040456ad532d 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -7749,16 +7749,15 @@ 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 bad_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, - ofperr_get_name(error), ds_cstr(&s)); + VLOG_WARN("bad action at offset %"PRIuSIZE" (%s):\n%s", + bad_action_offset, ofperr_get_name(error), ds_cstr(&s)); ds_destroy(&s); } } @@ -7769,25 +7768,46 @@ ofpacts_decode(const void *actions, size_t actions_len, const struct vl_mff_map *vl_mff_map, uint64_t *ofpacts_tlv_bitmap, struct ofpbuf *ofpacts) { - struct ofpbuf openflow = ofpbuf_const_initializer(actions, actions_len); - while (openflow.size) { - const struct ofp_action_header *action = openflow.data; + struct ofpbuf openflow_actions + = ofpbuf_const_initializer(actions, actions_len); + struct ofpbuf *openflow = &openflow_actions; + + enum ofperr error = 0; + while (openflow->size) { + /* Ensure the next action data is properly aligned. Trimming removes + * all headroom and ensures alignment. The headroom is also removed + * when cloning and ofpbuf. + */ + if (!OFPACT_IS_ALIGNED(openflow->data)) { + if (openflow == &openflow_actions) { + openflow = ofpbuf_clone(openflow); + } else { + ofpbuf_trim(openflow); + } + } + + const struct ofp_action_header *action = openflow->data; + size_t action_offset = actions_len - openflow->size; 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; + log_bad_action(actions, actions_len, action_offset, error); + goto done; } } - return 0; + +done: + if (openflow != &openflow_actions) { + ofpbuf_delete(openflow); + } + return error; } static enum ofperr diff --git a/lib/ofp-errors.c b/lib/ofp-errors.c index 35191abf16dd..eb099d355270 100644 --- a/lib/ofp-errors.c +++ b/lib/ofp-errors.c @@ -20,6 +20,7 @@ #include "openflow/openflow.h" #include "openflow/nicira-ext.h" #include "openvswitch/dynamic-string.h" +#include "openvswitch/ofp-actions.h" #include "openvswitch/ofp-errors.h" #include "openvswitch/ofp-msgs.h" #include "openvswitch/ofp-print.h" @@ -346,6 +347,7 @@ ofperr_decode_msg(const struct ofp_header *oh, struct ofpbuf *payload) if (error && payload) { ofpbuf_init(payload, b.size); ofpbuf_push(payload, b.data, b.size); + ofpbuf_trim(payload); } return error; } diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 41461e802fb5..991e22e6b64b 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -7034,10 +7034,15 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, /* Set the field only if the packet actually has it. */ if (mf_are_prereqs_ok(mf, flow, wc)) { - mf_mask_field_masked(mf, ofpact_set_field_mask(set_field), wc); - mf_set_flow_value_masked(mf, set_field->value, - ofpact_set_field_mask(set_field), - flow); + union mf_value *mask = ofpact_set_field_mask(set_field); + union mf_value aligned_mask; + + if (!IS_PTR_ALIGNED(mask)) { + memcpy(&aligned_mask, mask, mf->n_bytes); + mask = &aligned_mask; + } + mf_mask_field_masked(mf, mask, wc); + mf_set_flow_value_masked(mf, set_field->value, mask, flow); } else { xlate_report(ctx, OFT_WARN, "unmet prerequisites for %s, set_field ignored",
When decoding multiple actions in ofpacts_decode(), make sure that when we advance to the next action it will be properly aligned (multiple of OFPACT_ALIGNTO). If not, clone or trim the ofpbuf to ensure proper aligment. One example is parsing the OVN "eth.dst[40] = 1;" action, which triggered the following warning from UndefinedBehaviorSanitizer: lib/meta-flow.c:3210:9: runtime error: member access within misaligned address 0x000000de4e36 for type 'const union mf_value', which requires 8 byte alignment 0x000000de4e36: note: pointer points here 00 00 00 00 01 00 00 00 00 00 00 00 00 00 70 4e de 00 00 00 00 00 10 51 de 00 00 00 00 00 c0 4f ^ #0 0x5818bc in mf_format lib/meta-flow.c:3210 #1 0x5b6047 in format_SET_FIELD lib/ofp-actions.c:3342 #2 0x5d68ab in ofpact_format lib/ofp-actions.c:9213 #3 0x5d6ee0 in ofpacts_format lib/ofp-actions.c:9237 #4 0x410922 in test_parse_actions tests/test-ovn.c:1360 Another example is 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 [...] Or: lib/ofp-print.c:1218:24: runtime error: member access within misaligned address 0x0000019229d2 for type 'const struct ofp_header', which requires 4 byte alignment 0x0000019229d2: note: pointer points here 00 00 5a 5a 05 22 00 3e 00 00 00 09 00 00 00 00 00 00 00 03 05 0d 00 2e 00 00 00 09 ff ff ff ff ^ #0 0x7d45cc in ofp_to_string lib/ofp-print.c:1218 #1 0x774fa8 in ofperr_msg_format lib/ofp-errors.c:253 #2 0x7d2617 in ofp_print_error_msg lib/ofp-print.c:435 #3 0x7d3eb7 in ofp_to_string__ lib/ofp-print.c:998 #4 0x7d47fb in ofp_to_string lib/ofp-print.c:1244 #5 0x8dcc4b in do_send lib/vconn.c:688 #6 0x8dca64 in vconn_send lib/vconn.c:671 [...] Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- include/openvswitch/ofp-actions.h | 1 + include/openvswitch/util.h | 3 +++ lib/meta-flow.c | 6 +++++ lib/nx-match.c | 7 ++++++ lib/ofp-actions.c | 44 +++++++++++++++++++++++++++---------- lib/ofp-errors.c | 2 ++ ofproto/ofproto-dpif-xlate.c | 13 ++++++++--- 7 files changed, 60 insertions(+), 16 deletions(-)