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