From patchwork Mon Oct 19 20:03:00 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Maximets X-Patchwork-Id: 1384497 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.137; helo=fraxinus.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4CFSP14qlHz9sSC for ; Tue, 20 Oct 2020 07:03:17 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id C795B86F4E; Mon, 19 Oct 2020 20:03:15 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id AefEXAH-0Ybu; Mon, 19 Oct 2020 20:03:15 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by fraxinus.osuosl.org (Postfix) with ESMTP id 1FEC986F42; Mon, 19 Oct 2020 20:03:15 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 0D0B9C0052; Mon, 19 Oct 2020 20:03:15 +0000 (UTC) X-Original-To: ovs-dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) by lists.linuxfoundation.org (Postfix) with ESMTP id 46C96C0051 for ; Mon, 19 Oct 2020 20:03:13 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id 2D9C086F42 for ; Mon, 19 Oct 2020 20:03:13 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id kM4XUj3ciolj for ; Mon, 19 Oct 2020 20:03:12 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net [217.70.183.199]) by fraxinus.osuosl.org (Postfix) with ESMTPS id C2CD086F4D for ; Mon, 19 Oct 2020 20:03:11 +0000 (UTC) X-Originating-IP: 78.45.89.65 Received: from im-t490s.redhat.com (ip-78-45-89-65.net.upcbroadband.cz [78.45.89.65]) (Authenticated sender: i.maximets@ovn.org) by relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 17077FF804; Mon, 19 Oct 2020 20:03:07 +0000 (UTC) From: Ilya Maximets To: ovs-dev@openvswitch.org Date: Mon, 19 Oct 2020 22:03:00 +0200 Message-Id: <20201019200300.3140350-1-i.maximets@ovn.org> X-Mailer: git-send-email 2.25.4 MIME-Version: 1.0 Cc: Ilya Maximets Subject: [ovs-dev] [PATCH] odp-util: Fix overflow of nested netlink attributes. 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" Length of nested attributes must be checked before storing to the header. If current length exceeds the maximum value parsing should fail, otherwise the length value will be truncated leading to corrupted netlink message and out-of-bound memory accesses: ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6310002cc838 at pc 0x000000575470 bp 0x7ffc6c322d60 sp 0x7ffc6c322d58 READ of size 1 at 0x6310002cc838 thread T0 SCARINESS: 12 (1-byte-read-heap-buffer-overflow) #0 0x57546f in format_generic_odp_key lib/odp-util.c:2738:39 #1 0x559e70 in check_attr_len lib/odp-util.c:3572:13 #2 0x56581a in format_odp_key_attr lib/odp-util.c:4392:9 #3 0x5563b9 in format_odp_action lib/odp-util.c:1192:9 #4 0x555d75 in format_odp_actions lib/odp-util.c:1279:13 ... Fix that by checking the length of nested netlink attributes before updating 'nla_len' inside the header. Additionally introduced assertion inside nl_msg_end_nested() to catch this kind of issues before actual overflow happened. Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20003 Fixes: 65da723b40a5 ("odp-util: Format tunnel attributes directly from netlink.") Signed-off-by: Ilya Maximets Acked-by: Flavio Leitner --- lib/netlink.c | 1 + lib/odp-util.c | 17 ++++++++++------- tests/tunnel.at | 29 +++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 7 deletions(-) diff --git a/lib/netlink.c b/lib/netlink.c index de3ebcd0e..26ab20bb4 100644 --- a/lib/netlink.c +++ b/lib/netlink.c @@ -498,6 +498,7 @@ void nl_msg_end_nested(struct ofpbuf *msg, size_t offset) { struct nlattr *attr = ofpbuf_at_assert(msg, offset, sizeof *attr); + ovs_assert(!nl_attr_oversized(msg->size - offset - NLA_HDRLEN)); attr->nla_len = msg->size - offset; } diff --git a/lib/odp-util.c b/lib/odp-util.c index 0bd2f9aa8..252a91bfa 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -5557,13 +5557,16 @@ gtpu_to_attr(struct ofpbuf *a, const void *data_) do { \ len = 0; -#define SCAN_END_NESTED() \ - SCAN_FINISH(); \ - nl_msg_end_nested(key, key_offset); \ - if (mask) { \ - nl_msg_end_nested(mask, mask_offset); \ - } \ - return s - start; \ +#define SCAN_END_NESTED() \ + SCAN_FINISH(); \ + if (nl_attr_oversized(key->size - key_offset - NLA_HDRLEN)) { \ + return -E2BIG; \ + } \ + nl_msg_end_nested(key, key_offset); \ + if (mask) { \ + nl_msg_end_nested(mask, mask_offset); \ + } \ + return s - start; \ } #define SCAN_FIELD_NESTED__(NAME, TYPE, SCAN_AS, ATTR, FUNC) \ diff --git a/tests/tunnel.at b/tests/tunnel.at index e08fd1e04..b8ae7caa9 100644 --- a/tests/tunnel.at +++ b/tests/tunnel.at @@ -132,6 +132,35 @@ tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1,tp_dst=123),recir OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([tunnel - too long nested attributes]) +OVS_VSWITCHD_START([add-port br0 p1 \ + -- set Interface p1 type=gre options:remote_ip=1.1.1.1 ofport_request=1 \ + -- add-port br0 p2 -- set Interface p2 type=dummy ofport_request=2]) + +AT_CHECK([ovs-appctl dpif/show | tail -n +3], [0], [dnl + br0 65534/100: (dummy-internal) + p1 1/1: (gre: remote_ip=1.1.1.1) + p2 2/2: (dummy) +]) + +dst_single="dst=1.1.1.1" +dst_rep=${dst_single} +dnl Size of one OVS_TUNNEL_KEY_ATTR_IPV4_DST is 4 bytes + NLA_HDRLEN (4 bytes). +dnl One nested message has room for UINT16_MAX - NLA_HDRLEN (4) bytes, i.e. +dnl (UINT16_MAX - NLA_HDRLEN) / (4 + NLA_HDRLEN) = 8191.375 of dst addresses. +for i in `seq 1 8192` ; do + dst_rep="${dst_rep},${dst_single}" +done + +AT_CHECK([ovs-appctl dpctl/add-flow "tunnel(${dst_rep})" "2" 2>&1 | dnl + sed "s/${dst_single},//g"], [], [dnl +ovs-vswitchd: parsing flow key (syntax error at tunnel(dst=1.1.1.1)) (Argument list too long) +ovs-appctl: ovs-vswitchd: server returned an error +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([tunnel - output]) OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=gre \ options:remote_ip=1.1.1.1 options:local_ip=2.2.2.2 \