Skip to content

Commit

Permalink
ofp-actions: Fix use of uninitialized padding in set-field.
Browse files Browse the repository at this point in the history
Commit 933aaf9 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: 933aaf9 ("ofp-actions: Ensure aligned accesses to masked fields.")
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
  • Loading branch information
igsilya committed Nov 27, 2024
1 parent 2af7cef commit b5c3651
Showing 1 changed file with 7 additions and 7 deletions.
14 changes: 7 additions & 7 deletions lib/ofp-actions.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down

0 comments on commit b5c3651

Please sign in to comment.