Dumitru Ceara <[email protected]> writes:
> On 4/5/22 16:41, Aaron Conole wrote:
>> Dumitru Ceara <[email protected]> writes:
>>
>>> This is undefined behavior and was reported by UB Sanitizer:
>>> lib/meta-flow.c:3445:16: runtime error: member access within null pointer
>>> of type 'struct vl_mf_field'
>>> #0 0x6aad0f in mf_get_vl_mff lib/meta-flow.c:3445
>>> #1 0x6d96d7 in mf_from_oxm_header lib/nx-match.c:260
>>> #2 0x6d9e2e in nx_pull_header__ lib/nx-match.c:341
>>> #3 0x6daafa in nx_pull_header lib/nx-match.c:488
>>> #4 0x6abcb6 in mf_vl_mff_nx_pull_header lib/meta-flow.c:3605
>>> #5 0x73b9be in decode_NXAST_RAW_REG_MOVE lib/ofp-actions.c:2652
>>> #6 0x764ccd in ofpact_decode lib/ofp-actions.inc2:4681
>>> [...]
>>> lib/sset.c:315:12: runtime error: applying zero offset to null pointer
>>> #0 0xcc2e6a in sset_at_position /root/ovs/lib/sset.c:315:12
>>> #1 0x5734b3 in port_dump_next /root/ovs/ofproto/ofproto-dpif.c:4083:20
>>> [...]
>>> lib/ovsdb-data.c:2194:56: runtime error: applying zero offset to null
>>> pointer
>>> #0 0x5e9530 in ovsdb_datum_added_removed
>>> /root/ovs/lib/ovsdb-data.c:2194:56
>>> #1 0x4d6258 in update_row_ref_count
>>> /root/ovs/ovsdb/transaction.c:335:17
>>> #2 0x4c360b in for_each_txn_row /root/ovs/ovsdb/transaction.c:1572:33
>>> [...]
>>> lib/ofpbuf.c:440:30: runtime error: applying zero offset to null pointer
>>> #0 0x75066d in ofpbuf_push_uninit lib/ofpbuf.c:440
>>> #1 0x46ac8a in ovnacts_parse lib/actions.c:4190
>>> #2 0x46ad91 in ovnacts_parse_string lib/actions.c:4208
>>> #3 0x4106d1 in test_parse_actions tests/test-ovn.c:1324
>>> [...]
>>> lib/ofp-actions.c:3205:22: runtime error: applying non-zero offset 2 to
>>> null pointer
>>> #0 0x6e1641 in set_field_split_str /root/ovs/lib/ofp-actions.c:3205:22
>>> [...]
>>> lib/tnl-ports.c:74:12: runtime error: applying zero offset to null pointer
>>> #0 0xceffe7 in tnl_port_cast /root/ovs/lib/tnl-ports.c:74:12
>>> #1 0xcf14c3 in map_insert /root/ovs/lib/tnl-ports.c:116:13
>>> [...]
>>> ofproto/ofproto.c:8905:16: runtime error: applying zero offset to null
>>> pointer
>>> #0 0x556795 in eviction_group_hash_rule
>>> /root/ovs/ofproto/ofproto.c:8905:16
>>> #1 0x503f8d in eviction_group_add_rule
>>> /root/ovs/ofproto/ofproto.c:9022:42
>>> [...]
>>>
>>> Also, it's valid to have an empty ofpact list and we should be able to
>>> try to iterate through it.
>>>
>>> UB Sanitizer report:
>>> include/openvswitch/ofp-actions.h:222:12: runtime error: applying zero
>>> offset to null pointer
>>> #0 0x665d69 in ofpact_end
>>> /root/ovs/./include/openvswitch/ofp-actions.h:222:12
>>> #1 0x66b2cf in ofpacts_put_openflow_actions
>>> /root/ovs/lib/ofp-actions.c:8861:5
>>> #2 0x6ffdd1 in ofputil_encode_flow_mod /root/ovs/lib/ofp-flow.c:447:9
>>> [...]
>>>
>>> Signed-off-by: Dumitru Ceara <[email protected]>
>>> ---
>>> v5:
>>> - Rebase.
>>> v4:
>>> - Addressed Ilya's comments.
>>> ---
>>
>> Glad to see that the undefined behavior got removed, BUT this
>> can introduce some different undefined behavior - places where we
>> have a calls to ofpbuf_at_...() always assume a valid pointer is
>> returned.
>>
>
> Thanks for the review!
>
>> I think it makes sense to abort if b->data is NULL in these cases.
>> Maybe something like:
>>
>> ovs_abort(0, "invalid buffer data pointer");
>>
>> WDYT?
>>
>
> Calling ovs_abort() directly from openvswitch/util.h will be a challenge
> because it's an internal function and the openvswitch/util.h header is
> public. Worst case we just call ovs_assert() like we already do in
> ofpbuf_at_assert().
Maybe we can expose ovs_abort as well?
> But, just to make sure I understood properly, you'd like to assert that
> b->data is not NULL only in ofpbuf_at() and ofpbuf_at_assert(), right?
right - only for those places where we have the assumption that the
return must be !NULL
> Because the other ofpact_...() functions are also called in valid
> scenarios on ofpbufs that have b->data = NULL.
>
>>> include/openvswitch/ofp-actions.h | 4 +++-
>>> include/openvswitch/ofpbuf.h | 17 ++++++++++++-----
>>> lib/dynamic-string.c | 8 ++++++--
>>> lib/meta-flow.c | 4 +++-
>>> lib/ofp-actions.c | 8 ++++----
>>> lib/ofpbuf.c | 4 ++++
>>> lib/ovsdb-data.c | 37
>>> +++++++++++++++++++++----------------
>>> lib/ovsdb-data.h | 4 ++++
>>> lib/sset.c | 4 +++-
>>> lib/tnl-ports.c | 2 +-
>>> ofproto/ofproto.c | 2 +-
>>> 11 files changed, 62 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/include/openvswitch/ofp-actions.h
>>> b/include/openvswitch/ofp-actions.h
>>> index 41bcb55d2056..b7231c7bb334 100644
>>> --- a/include/openvswitch/ofp-actions.h
>>> +++ b/include/openvswitch/ofp-actions.h
>>> @@ -218,7 +218,9 @@ struct ofpact *ofpact_next_flattened(const struct
>>> ofpact *);
>>> static inline struct ofpact *
>>> ofpact_end(const struct ofpact *ofpacts, size_t ofpacts_len)
>>> {
>>> - return ALIGNED_CAST(struct ofpact *, (uint8_t *) ofpacts +
>>> ofpacts_len);
>>> + return ofpacts
>>> + ? ALIGNED_CAST(struct ofpact *, (uint8_t *) ofpacts +
>>> ofpacts_len)
>>> + : NULL;
>>> }
>>>
>>> static inline bool
>>> diff --git a/include/openvswitch/ofpbuf.h b/include/openvswitch/ofpbuf.h
>>> index 1136ba04c84e..7b6aba9dc29c 100644
>>> --- a/include/openvswitch/ofpbuf.h
>>> +++ b/include/openvswitch/ofpbuf.h
>>> @@ -179,7 +179,9 @@ static inline void ofpbuf_delete(struct ofpbuf *b)
>>> static inline void *ofpbuf_at(const struct ofpbuf *b, size_t offset,
>>> size_t size)
>>> {
>>> - return offset + size <= b->size ? (char *) b->data + offset : NULL;
>>> + return (offset + size <= b->size) && b->data
>>> + ? (char *) b->data + offset
>>> + : NULL;
>>> }
>>>
>>> /* Returns a pointer to byte 'offset' in 'b', which must contain at least
>>> @@ -188,20 +190,20 @@ static inline void *ofpbuf_at_assert(const struct
>>> ofpbuf *b, size_t offset,
>>> size_t size)
>>> {
>>> ovs_assert(offset + size <= b->size);
>>> - return ((char *) b->data) + offset;
>>> + return b->data ? (char *) b->data + offset : NULL;
>>> }
>>>
>>> /* Returns a pointer to byte following the last byte of data in use in
>>> 'b'. */
>>> static inline void *ofpbuf_tail(const struct ofpbuf *b)
>>> {
>>> - return (char *) b->data + b->size;
>>> + return b->data ? (char *) b->data + b->size : NULL;
>>> }
>>>
>>> /* Returns a pointer to byte following the last byte allocated for use (but
>>> * not necessarily in use) in 'b'. */
>>> static inline void *ofpbuf_end(const struct ofpbuf *b)
>>> {
>>> - return (char *) b->base + b->allocated;
>>> + return b->base ? (char *) b->base + b->allocated : NULL;
>>> }
>>>
>>> /* Returns the number of bytes of headroom in 'b', that is, the number of
>>> bytes
>>> @@ -249,6 +251,11 @@ static inline void *ofpbuf_pull(struct ofpbuf *b,
>>> size_t size)
>>> {
>>> ovs_assert(b->size >= size);
>>> void *data = b->data;
>>> +
>>> + if (!size) {
>>> + return data;
>>> + }
>>> +
>>> b->data = (char*)b->data + size;
>>> b->size = b->size - size;
>>> return data;
>>> @@ -270,7 +277,7 @@ static inline struct ofpbuf *ofpbuf_from_list(const
>>> struct ovs_list *list)
>>> static inline bool ofpbuf_equal(const struct ofpbuf *a, const struct
>>> ofpbuf *b)
>>> {
>>> return a->size == b->size &&
>>> - memcmp(a->data, b->data, a->size) == 0;
>>> + (a->size == 0 || memcmp(a->data, b->data, a->size) == 0);
>>> }
>>>
>>> static inline bool ofpbuf_oversized(const struct ofpbuf *ofpacts)
>>> diff --git a/lib/dynamic-string.c b/lib/dynamic-string.c
>>> index fd0127ed1740..6940e1fd63bd 100644
>>> --- a/lib/dynamic-string.c
>>> +++ b/lib/dynamic-string.c
>>> @@ -152,7 +152,10 @@ ds_put_format_valist(struct ds *ds, const char
>>> *format, va_list args_)
>>>
>>> va_copy(args, args_);
>>> available = ds->string ? ds->allocated - ds->length + 1 : 0;
>>> - needed = vsnprintf(&ds->string[ds->length], available, format, args);
>>> + needed = vsnprintf(ds->string
>>> + ? &ds->string[ds->length]
>>> + : NULL,
>>> + available, format, args);
>>> va_end(args);
>>>
>>> if (needed < available) {
>>> @@ -162,7 +165,8 @@ ds_put_format_valist(struct ds *ds, const char *format,
>>> va_list args_)
>>>
>>> va_copy(args, args_);
>>> available = ds->allocated - ds->length + 1;
>>> - needed = vsnprintf(&ds->string[ds->length], available, format,
>>> args);
>>> + needed = vsnprintf(&ds->string[ds->length],
>>> + available, format, args);
>>> va_end(args);
>>>
>>> ovs_assert(needed < available);
>>> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
>>> index e03cd8d0c5cd..c576ae6202a4 100644
>>> --- a/lib/meta-flow.c
>>> +++ b/lib/meta-flow.c
>>> @@ -3442,7 +3442,9 @@ mf_get_vl_mff(const struct mf_field *mff,
>>> const struct vl_mff_map *vl_mff_map)
>>> {
>>> if (mff && mff->variable_len && vl_mff_map) {
>>> - return &mf_get_vl_mff__(mff->id, vl_mff_map)->mf;
>>> + struct vl_mf_field *vl_mff = mf_get_vl_mff__(mff->id, vl_mff_map);
>>> +
>>> + return vl_mff ? &vl_mff->mf : NULL;
>>> }
>>>
>>> return NULL;
>>> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
>>> index a0b70a89d780..4ada0f4a3c49 100644
>>> --- a/lib/ofp-actions.c
>>> +++ b/lib/ofp-actions.c
>>> @@ -3204,14 +3204,14 @@ set_field_split_str(char *arg, char **key, char
>>> **value, char **delim)
>>>
>>> *value = arg;
>>> value_end = strstr(arg, "->");
>>> + if (!value_end) {
>>> + return xasprintf("%s: missing `->'", arg);
>>> + }
>>> +
>>> *key = value_end + strlen("->");
>>> if (delim) {
>>> *delim = value_end;
>>> }
>>> -
>>> - if (!value_end) {
>>> - return xasprintf("%s: missing `->'", arg);
>>> - }
>>> if (strlen(value_end) <= strlen("->")) {
>>> return xasprintf("%s: missing field name following `->'", arg);
>>> }
>>> diff --git a/lib/ofpbuf.c b/lib/ofpbuf.c
>>> index 271105bdea17..0330681b2518 100644
>>> --- a/lib/ofpbuf.c
>>> +++ b/lib/ofpbuf.c
>>> @@ -436,6 +436,10 @@ ofpbuf_reserve(struct ofpbuf *b, size_t size)
>>> void *
>>> ofpbuf_push_uninit(struct ofpbuf *b, size_t size)
>>> {
>>> + if (!size) {
>>> + return b->data;
>>> + }
>>> +
>>> ofpbuf_prealloc_headroom(b, size);
>>> b->data = (char*)b->data - size;
>>> b->size += size;
>>> diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c
>>> index 6b1c20ff85a0..61ad7679a6c5 100644
>>> --- a/lib/ovsdb-data.c
>>> +++ b/lib/ovsdb-data.c
>>> @@ -1957,6 +1957,19 @@ ovsdb_datum_add_unsafe(struct ovsdb_datum *datum,
>>> }
>>> }
>>>
>>> +void
>>> +ovsdb_datum_add_from_index_unsafe(struct ovsdb_datum *dst,
>>> + const struct ovsdb_datum *src,
>>> + size_t idx,
>>> + const struct ovsdb_type *type)
>>> +{
>>> + const union ovsdb_atom *key = &src->keys[idx];
>>> + const union ovsdb_atom *value = type->value.type != OVSDB_TYPE_VOID
>>> + ? &src->values[idx]
>>> + : NULL;
>>> + ovsdb_datum_add_unsafe(dst, key, value, type, NULL);
>>> +}
>>> +
>>> /* Adds 'n' atoms starting from index 'start_idx' from 'src' to the end of
>>> * 'dst'. 'dst' should have enough memory allocated to hold the additional
>>> * 'n' atoms. Atoms are not cloned, i.e. 'dst' will reference the same
>>> data.
>>> @@ -2165,12 +2178,10 @@ ovsdb_datum_added_removed(struct ovsdb_datum *added,
>>> int c = ovsdb_atom_compare_3way(&old->keys[oi], &new->keys[ni],
>>> type->key.type);
>>> if (c < 0) {
>>> - ovsdb_datum_add_unsafe(removed, &old->keys[oi],
>>> &old->values[oi],
>>> - type, NULL);
>>> + ovsdb_datum_add_from_index_unsafe(removed, old, oi, type);
>>> oi++;
>>> } else if (c > 0) {
>>> - ovsdb_datum_add_unsafe(added, &new->keys[ni], &new->values[ni],
>>> - type, NULL);
>>> + ovsdb_datum_add_from_index_unsafe(added, new, ni, type);
>>> ni++;
>>> } else {
>>> if (type->value.type != OVSDB_TYPE_VOID &&
>>> @@ -2186,13 +2197,11 @@ ovsdb_datum_added_removed(struct ovsdb_datum *added,
>>> }
>>>
>>> for (; oi < old->n; oi++) {
>>> - ovsdb_datum_add_unsafe(removed, &old->keys[oi], &old->values[oi],
>>> - type, NULL);
>>> + ovsdb_datum_add_from_index_unsafe(removed, old, oi, type);
>>> }
>>>
>>> for (; ni < new->n; ni++) {
>>> - ovsdb_datum_add_unsafe(added, &new->keys[ni], &new->values[ni],
>>> - type, NULL);
>>> + ovsdb_datum_add_from_index_unsafe(added, new, ni, type);
>>> }
>>> }
>>>
>>> @@ -2228,12 +2237,10 @@ ovsdb_datum_diff(struct ovsdb_datum *diff,
>>> int c = ovsdb_atom_compare_3way(&old->keys[oi], &new->keys[ni],
>>> type->key.type);
>>> if (c < 0) {
>>> - ovsdb_datum_add_unsafe(diff, &old->keys[oi], &old->values[oi],
>>> - type, NULL);
>>> + ovsdb_datum_add_from_index_unsafe(diff, old, oi, type);
>>> oi++;
>>> } else if (c > 0) {
>>> - ovsdb_datum_add_unsafe(diff, &new->keys[ni], &new->values[ni],
>>> - type, NULL);
>>> + ovsdb_datum_add_from_index_unsafe(diff, new, ni, type);
>>> ni++;
>>> } else {
>>> if (type->value.type != OVSDB_TYPE_VOID &&
>>> @@ -2247,13 +2254,11 @@ ovsdb_datum_diff(struct ovsdb_datum *diff,
>>> }
>>>
>>> for (; oi < old->n; oi++) {
>>> - ovsdb_datum_add_unsafe(diff, &old->keys[oi], &old->values[oi],
>>> - type, NULL);
>>> + ovsdb_datum_add_from_index_unsafe(diff, old, oi, type);
>>> }
>>>
>>> for (; ni < new->n; ni++) {
>>> - ovsdb_datum_add_unsafe(diff, &new->keys[ni], &new->values[ni],
>>> - type, NULL);
>>> + ovsdb_datum_add_from_index_unsafe(diff, new, ni, type);
>>> }
>>> }
>>>
>>> diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h
>>> index 47115a7b85b7..ba5d179a6509 100644
>>> --- a/lib/ovsdb-data.h
>>> +++ b/lib/ovsdb-data.h
>>> @@ -280,6 +280,10 @@ void ovsdb_datum_add_unsafe(struct ovsdb_datum *,
>>> const union ovsdb_atom *value,
>>> const struct ovsdb_type *,
>>> const union ovsdb_atom *range_end_atom);
>>> +void ovsdb_datum_add_from_index_unsafe(struct ovsdb_datum *dst,
>>> + const struct ovsdb_datum *src,
>>> + size_t idx,
>>> + const struct ovsdb_type *type);
>>>
>>> /* Transactions with named-uuid row names. */
>>> struct json *ovsdb_datum_to_json_with_row_names(const struct ovsdb_datum *,
>>> diff --git a/lib/sset.c b/lib/sset.c
>>> index c3197e305fff..6fbaa9d60d85 100644
>>> --- a/lib/sset.c
>>> +++ b/lib/sset.c
>>> @@ -312,7 +312,9 @@ sset_at_position(const struct sset *set, struct
>>> sset_position *pos)
>>> struct hmap_node *hmap_node;
>>>
>>> hmap_node = hmap_at_position(&set->map, &pos->pos);
>>> - return SSET_NODE_FROM_HMAP_NODE(hmap_node);
>>> + return hmap_node
>>> + ? SSET_NODE_FROM_HMAP_NODE(hmap_node)
>>> + : NULL;
>>> }
>>>
>>> /* Replaces 'a' by the intersection of 'a' and 'b'. That is, removes from
>>> 'a'
>>> diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c
>>> index f9fee3793992..050eafa6b8c3 100644
>>> --- a/lib/tnl-ports.c
>>> +++ b/lib/tnl-ports.c
>>> @@ -71,7 +71,7 @@ tnl_port_cast(const struct cls_rule *cr)
>>> {
>>> BUILD_ASSERT_DECL(offsetof(struct tnl_port_in, cr) == 0);
>>>
>>> - return CONTAINER_OF(cr, struct tnl_port_in, cr);
>>> + return cr ? CONTAINER_OF(cr, struct tnl_port_in, cr) : NULL;
>>> }
>>>
>>> static void
>>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>>> index 2ed107800748..933f7de2dc56 100644
>>> --- a/ofproto/ofproto.c
>>> +++ b/ofproto/ofproto.c
>>> @@ -8963,7 +8963,7 @@ eviction_group_hash_rule(struct rule *rule)
>>> hash = table->eviction_group_id_basis;
>>> miniflow_expand(rule->cr.match.flow, &flow);
>>> for (sf = table->eviction_fields;
>>> - sf < &table->eviction_fields[table->n_eviction_fields];
>>> + sf && sf < &table->eviction_fields[table->n_eviction_fields];
>>> sf++)
>>> {
>>> if (mf_are_prereqs_ok(sf->field, &flow, NULL)) {
>>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
