Commit 933aaf9444a6 re-aligned the fields, so the access to them is
aligned, but it also didn't initialize the added padding. 'ofpacts'
are frequently compared with memcmp() and being hashed as part of the
frozen state causing false negative comparisons and potentially frozen
state lookup failures.
Found by running make check-valgrind on 'continuation - data stack'
tests:
Conditional jump or move depends on uninitialised value(s)
at 0x4EBC82: mhash_add__ (hash.h:66)
by 0x4EBC48: mhash_add (hash.h:78)
by 0x4EB4F8: hash_add (hash.h:109)
by 0x4EBDEC: hash_add64 (hash.h:114)
by 0x4EBDAC: hash_add_words64 (hash.h:439)
by 0x4EB6D2: hash_words64_inline (hash.h:136)
by 0x4EB6A2: hash_words64__ (hash.c:73)
by 0x4595F2: hash_words64 (hash.h:371)
by 0x4593C6: hash_bytes64 (hash.h:399)
by 0x458B76: frozen_state_hash (ofproto-dpif-rid.c:143)
by 0x458CA4: recirc_alloc_id_ctx (ofproto-dpif-rid.c:280)
by 0x483B85: finish_freezing__ (ofproto-dpif-xlate.c:5229)
by 0x47171F: finish_freezing (ofproto-dpif-xlate.c:5271)
by 0x46E8BB: xlate_actions (ofproto-dpif-xlate.c:8340)
by 0x45DC7B: ofproto_trace__ (ofproto-dpif-trace.c:782)
by 0x45D816: ofproto_trace (ofproto-dpif-trace.c:851)
by 0x45E435: ofproto_unixctl_trace (ofproto-dpif-trace.c:490)
by 0x609F5E: process_command (unixctl.c:310)
by 0x6094B9: run_connection (unixctl.c:344)
by 0x609397: unixctl_server_run (unixctl.c:395)
Uninitialised value was created by a stack allocation
at 0x432A44: handle_flow_mod (ofproto.c:6346)
Fix that by properly initializing the whole space allocated for the
set-field action.
Fixes: 933aaf9444a6 ("ofp-actions: Ensure aligned accesses to masked fields.")
Signed-off-by: Ilya Maximets <[email protected]>
---
lib/ofp-actions.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index fe6a17b6d..8a05f7c9c 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -3402,21 +3402,21 @@ ofpact_put_set_field(struct ofpbuf *ofpacts, const
struct mf_field *field,
struct ofpact_set_field *sf = ofpact_put_SET_FIELD(ofpacts);
sf->field = field;
- /* Fill in the value and mask if given, otherwise put zeroes so that the
- * caller may fill in the value and mask itself. */
+ /* Fill with all zeroes to make sure the padding is initialized. */
+ ofpbuf_put_zeros(ofpacts, total_size);
+ sf = ofpacts->header;
+
+ /* Fill in the value and mask if given, otherwise keep the zeroes
+ * so that the caller may fill in the value and mask itself. */
if (value) {
- ofpbuf_put_uninit(ofpacts, total_size);
- sf = ofpacts->header;
memcpy(sf->value, value, field->n_bytes);
if (mask) {
memcpy(ofpact_set_field_mask(sf), mask, field->n_bytes);
} else {
memset(ofpact_set_field_mask(sf), 0xff, field->n_bytes);
}
- } else {
- ofpbuf_put_zeros(ofpacts, total_size);
- sf = ofpacts->header;
}
+
/* Update length. */
ofpact_finish_SET_FIELD(ofpacts, &sf);
--
2.47.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
