From patchwork Mon Jan 24 14:19:06 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dumitru Ceara X-Patchwork-Id: 1583500 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: bilbo.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=T+kCljc8; dkim-atps=neutral Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=2605:bc80:3010::136; helo=smtp3.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Received: from smtp3.osuosl.org (smtp3.osuosl.org [IPv6:2605:bc80:3010::136]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by bilbo.ozlabs.org (Postfix) with ESMTPS id 4JjBvf3Jv2z9t5m for ; Tue, 25 Jan 2022 01:19:58 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id 93BE260EA0; Mon, 24 Jan 2022 14:19:55 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id cqX6tNAX1l3D; Mon, 24 Jan 2022 14:19:54 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp3.osuosl.org (Postfix) with ESMTPS id 4AB2560E8A; Mon, 24 Jan 2022 14:19:53 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id F16F9C0039; Mon, 24 Jan 2022 14:19:52 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp4.osuosl.org (smtp4.osuosl.org [IPv6:2605:bc80:3010::137]) by lists.linuxfoundation.org (Postfix) with ESMTP id 03ABFC0039 for ; Mon, 24 Jan 2022 14:19:52 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 423D640946 for ; Mon, 24 Jan 2022 14:19:17 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Authentication-Results: smtp4.osuosl.org (amavisd-new); dkim=pass (1024-bit key) header.d=redhat.com Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id JfmMnxHLFOcZ for ; Mon, 24 Jan 2022 14:19:16 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by smtp4.osuosl.org (Postfix) with ESMTPS id 1A304416FD for ; Mon, 24 Jan 2022 14:19:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1643033955; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=7Br4Wkkb0tuq4SzBeKKkGk/HN6uWqH10bqCR1idm5hI=; b=T+kCljc8SoBpAdSwOf2ClbQMIwrFPFhtUplw+DpW7jV/qHnMBHtiyTZPABTVVfyGveA+J7 V7/VzkvnJSmaPtL1LCZzT9iXDJqxWUz4fEc8QewlhMeSXjdMhUe0HYvBckoNL9wNVjPm6o Ihmuz45dy43Z85wuXnC5SJlD1yz4GDY= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-612-BA1YWxC6PjWxa9IpJ0xmgg-1; Mon, 24 Jan 2022 09:19:11 -0500 X-MC-Unique: BA1YWxC6PjWxa9IpJ0xmgg-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D78D08144E3; Mon, 24 Jan 2022 14:19:10 +0000 (UTC) Received: from dceara.remote.csb (unknown [10.39.195.42]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5C1297B6ED; Mon, 24 Jan 2022 14:19:09 +0000 (UTC) From: Dumitru Ceara To: dev@openvswitch.org Date: Mon, 24 Jan 2022 15:19:06 +0100 Message-Id: <20220124141859.11777.68499.stgit@dceara.remote.csb> In-Reply-To: <20220124141706.11777.99804.stgit@dceara.remote.csb> References: <20220124141706.11777.99804.stgit@dceara.remote.csb> User-Agent: StGit/0.17.1-dirty MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dceara@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Cc: i.maximets@ovn.org Subject: [ovs-dev] [PATCH v3 08/14] treewide: Avoid offsetting NULL pointers. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" 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 --- include/openvswitch/ofp-actions.h | 4 +++- include/openvswitch/ofpbuf.h | 17 ++++++++++++----- lib/dynamic-string.c | 10 ++++++++-- 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, 64 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..a9ff8a56e81c 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..9b9a8f50e562 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,10 @@ 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->string[ds->length] + : NULL, + 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 006837c2e1f5..b24b46d2196b 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -3202,14 +3202,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..306b8d0d946a 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_unsafe_index(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_unsafe_index(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_unsafe_index(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_unsafe_index(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_unsafe_index(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_unsafe_index(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_unsafe_index(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_unsafe_index(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_unsafe_index(diff, new, ni, type); } } diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h index 47115a7b85b7..31c7c27e6b93 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_unsafe_index(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 b2e3f43ec91b..b18b948fa4e7 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 58269d3b1631..dd3320ccbb96 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 56aeac720935..c3ffbbb13b5b 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -8902,7 +8902,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)) {