Re: [ovs-dev] [PATCH 10/10] conntrack: Validate accessing of conntrack data in pkt_metadata
On Wed, Sep 11, 2019 at 02:18:36PM -0700, Yifeng Sun wrote:
> Valgrind reported:
>
> 1305: ofproto-dpif - conntrack - ipv6
>
> ==26942== Conditional jump or move depends on uninitialised value(s)
> ==26942==at 0x587C00: check_orig_tuple (conntrack.c:1006)
> ==26942==by 0x587C00: process_one (conntrack.c:1141)
> ==26942==by 0x587C00: conntrack_execute (conntrack.c:1220)
> ==26942==by 0x47B00F: dp_execute_cb (dpif-netdev.c:7305)
> ==26942==by 0x4AF756: odp_execute_actions (odp-execute.c:794)
> ==26942==by 0x477532: dp_netdev_execute_actions (dpif-netdev.c:7349)
> ==26942==by 0x477532: handle_packet_upcall (dpif-netdev.c:6630)
> ==26942==by 0x477532: fast_path_processing (dpif-netdev.c:6726)
> ==26942==by 0x47933C: dp_netdev_input__ (dpif-netdev.c:6814)
> ==26942==by 0x479AB8: dp_netdev_input (dpif-netdev.c:6852)
> ==26942==by 0x479AB8: dp_netdev_process_rxq_port (dpif-netdev.c:4287)
> ==26942==by 0x47A6A9: dpif_netdev_run (dpif-netdev.c:5264)
> ==26942==by 0x4324E7: type_run (ofproto-dpif.c:342)
> ==26942==by 0x41C5FE: ofproto_type_run (ofproto.c:1734)
> ==26942==by 0x40BAAC: bridge_run__ (bridge.c:2965)
> ==26942==by 0x410CF3: bridge_run (bridge.c:3029)
> ==26942==by 0x407614: main (ovs-vswitchd.c:127)
> ==26942== Uninitialised value was created by a heap allocation
> ==26942==at 0x4C2DB8F: malloc (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==26942==by 0x532574: xmalloc (util.c:138)
> ==26942==by 0x46CD62: dp_packet_new (dp-packet.c:153)
> ==26942==by 0x4A0431: eth_from_flow_str (netdev-dummy.c:1644)
> ==26942==by 0x4A0431: netdev_dummy_receive (netdev-dummy.c:1783)
> ==26942==by 0x531990: process_command (unixctl.c:308)
> ==26942==by 0x531990: run_connection (unixctl.c:342)
> ==26942==by 0x531990: unixctl_server_run (unixctl.c:393)
> ==26942==by 0x40761E: main (ovs-vswitchd.c:128)
>
> 1316: ofproto-dpif - conntrack - tcp port reuse
>
> ==24039== Conditional jump or move depends on uninitialised value(s)
> ==24039==at 0x587BF5: check_orig_tuple (conntrack.c:1004)
> ==24039==by 0x587BF5: process_one (conntrack.c:1141)
> ==24039==by 0x587BF5: conntrack_execute (conntrack.c:1220)
> ==24039==by 0x47B02F: dp_execute_cb (dpif-netdev.c:7306)
> ==24039==by 0x4AF7A6: odp_execute_actions (odp-execute.c:794)
> ==24039==by 0x47755B: dp_netdev_execute_actions (dpif-netdev.c:7350)
> ==24039==by 0x47755B: handle_packet_upcall (dpif-netdev.c:6631)
> ==24039==by 0x47755B: fast_path_processing (dpif-netdev.c:6727)
> ==24039==by 0x47935C: dp_netdev_input__ (dpif-netdev.c:6815)
> ==24039==by 0x479AD8: dp_netdev_input (dpif-netdev.c:6853)
> ==24039==by 0x479AD8: dp_netdev_process_rxq_port
> (dpif-netdev.c:4287)
> ==24039==by 0x47A6C9: dpif_netdev_run (dpif-netdev.c:5264)
> ==24039==by 0x4324F7: type_run (ofproto-dpif.c:342)
> ==24039==by 0x41C5FE: ofproto_type_run (ofproto.c:1734)
> ==24039==by 0x40BAAC: bridge_run__ (bridge.c:2965)
> ==24039==by 0x410CF3: bridge_run (bridge.c:3029)
> ==24039==by 0x407614: main (ovs-vswitchd.c:127)
> ==24039== Uninitialised value was created by a heap allocation
> ==24039==at 0x4C2DB8F: malloc (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==24039==by 0x5325C4: xmalloc (util.c:138)
> ==24039==by 0x46D144: dp_packet_new (dp-packet.c:153)
> ==24039==by 0x46D144: dp_packet_new_with_headroom (dp-packet.c:163)
> ==24039==by 0x51191E: eth_from_hex (packets.c:498)
> ==24039==by 0x4A03B9: eth_from_packet (netdev-dummy.c:1609)
> ==24039==by 0x4A03B9: netdev_dummy_receive (netdev-dummy.c:1765)
> ==24039==by 0x5319E0: process_command (unixctl.c:308)
> ==24039==by 0x5319E0: run_connection (unixctl.c:342)
> ==24039==by 0x5319E0: unixctl_server_run (unixctl.c:393)
> ==24039==by 0x40761E: main (ovs-vswitchd.c:128)
>
> According to comments in pkt_metadata_init(), conntrack data is valid
> only if pkt_metadata.ct_state != 0. This patch prevents
> check_orig_tuple() get called when conntrack data is uninitialized.
>
> Signed-off-by: Yifeng Sun
LGTM
Acked-by: William Tu
> ---
> lib/conntrack.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index e5266e579452..86c16b2fbe77 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -1138,7 +1138,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
> handle_nat(pkt, conn, zone, ctx->reply, ctx->icmp_related);
> }
>
> -} else if (check_orig_tuple(ct, pkt, ctx, now, , nat_action_info)) {
> +} else if (pkt->md.ct_state
> + && check_orig_tuple(ct, pkt, ctx, now, ,
> nat_action_info)) {
> create_new_conn = conn_update_state(ct, pkt, ctx, conn, now);
> } else {
> if (ctx->icmp_related) {
> --
> 2.7.4
>
> ___
> dev mailing list
Re: [ovs-dev] [PATCH 10/10] conntrack: Validate accessing of conntrack data in pkt_metadata
Bleep bloop. Greetings Yifeng Sun, I am a robot and I have tried out your
patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 80 characters long (recommended limit is 79)
#93 FILE: lib/conntrack.c:1142:
&& check_orig_tuple(ct, pkt, ctx, now, , nat_action_info)) {
Lines checked: 99, Warnings: 1, Errors: 0
Please check this out. If you feel there has been an error, please email
acon...@redhat.com
Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 10/10] conntrack: Validate accessing of conntrack data in pkt_metadata
Valgrind reported:
1305: ofproto-dpif - conntrack - ipv6
==26942== Conditional jump or move depends on uninitialised value(s)
==26942==at 0x587C00: check_orig_tuple (conntrack.c:1006)
==26942==by 0x587C00: process_one (conntrack.c:1141)
==26942==by 0x587C00: conntrack_execute (conntrack.c:1220)
==26942==by 0x47B00F: dp_execute_cb (dpif-netdev.c:7305)
==26942==by 0x4AF756: odp_execute_actions (odp-execute.c:794)
==26942==by 0x477532: dp_netdev_execute_actions (dpif-netdev.c:7349)
==26942==by 0x477532: handle_packet_upcall (dpif-netdev.c:6630)
==26942==by 0x477532: fast_path_processing (dpif-netdev.c:6726)
==26942==by 0x47933C: dp_netdev_input__ (dpif-netdev.c:6814)
==26942==by 0x479AB8: dp_netdev_input (dpif-netdev.c:6852)
==26942==by 0x479AB8: dp_netdev_process_rxq_port (dpif-netdev.c:4287)
==26942==by 0x47A6A9: dpif_netdev_run (dpif-netdev.c:5264)
==26942==by 0x4324E7: type_run (ofproto-dpif.c:342)
==26942==by 0x41C5FE: ofproto_type_run (ofproto.c:1734)
==26942==by 0x40BAAC: bridge_run__ (bridge.c:2965)
==26942==by 0x410CF3: bridge_run (bridge.c:3029)
==26942==by 0x407614: main (ovs-vswitchd.c:127)
==26942== Uninitialised value was created by a heap allocation
==26942==at 0x4C2DB8F: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==26942==by 0x532574: xmalloc (util.c:138)
==26942==by 0x46CD62: dp_packet_new (dp-packet.c:153)
==26942==by 0x4A0431: eth_from_flow_str (netdev-dummy.c:1644)
==26942==by 0x4A0431: netdev_dummy_receive (netdev-dummy.c:1783)
==26942==by 0x531990: process_command (unixctl.c:308)
==26942==by 0x531990: run_connection (unixctl.c:342)
==26942==by 0x531990: unixctl_server_run (unixctl.c:393)
==26942==by 0x40761E: main (ovs-vswitchd.c:128)
1316: ofproto-dpif - conntrack - tcp port reuse
==24039== Conditional jump or move depends on uninitialised value(s)
==24039==at 0x587BF5: check_orig_tuple (conntrack.c:1004)
==24039==by 0x587BF5: process_one (conntrack.c:1141)
==24039==by 0x587BF5: conntrack_execute (conntrack.c:1220)
==24039==by 0x47B02F: dp_execute_cb (dpif-netdev.c:7306)
==24039==by 0x4AF7A6: odp_execute_actions (odp-execute.c:794)
==24039==by 0x47755B: dp_netdev_execute_actions (dpif-netdev.c:7350)
==24039==by 0x47755B: handle_packet_upcall (dpif-netdev.c:6631)
==24039==by 0x47755B: fast_path_processing (dpif-netdev.c:6727)
==24039==by 0x47935C: dp_netdev_input__ (dpif-netdev.c:6815)
==24039==by 0x479AD8: dp_netdev_input (dpif-netdev.c:6853)
==24039==by 0x479AD8: dp_netdev_process_rxq_port
(dpif-netdev.c:4287)
==24039==by 0x47A6C9: dpif_netdev_run (dpif-netdev.c:5264)
==24039==by 0x4324F7: type_run (ofproto-dpif.c:342)
==24039==by 0x41C5FE: ofproto_type_run (ofproto.c:1734)
==24039==by 0x40BAAC: bridge_run__ (bridge.c:2965)
==24039==by 0x410CF3: bridge_run (bridge.c:3029)
==24039==by 0x407614: main (ovs-vswitchd.c:127)
==24039== Uninitialised value was created by a heap allocation
==24039==at 0x4C2DB8F: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==24039==by 0x5325C4: xmalloc (util.c:138)
==24039==by 0x46D144: dp_packet_new (dp-packet.c:153)
==24039==by 0x46D144: dp_packet_new_with_headroom (dp-packet.c:163)
==24039==by 0x51191E: eth_from_hex (packets.c:498)
==24039==by 0x4A03B9: eth_from_packet (netdev-dummy.c:1609)
==24039==by 0x4A03B9: netdev_dummy_receive (netdev-dummy.c:1765)
==24039==by 0x5319E0: process_command (unixctl.c:308)
==24039==by 0x5319E0: run_connection (unixctl.c:342)
==24039==by 0x5319E0: unixctl_server_run (unixctl.c:393)
==24039==by 0x40761E: main (ovs-vswitchd.c:128)
According to comments in pkt_metadata_init(), conntrack data is valid
only if pkt_metadata.ct_state != 0. This patch prevents
check_orig_tuple() get called when conntrack data is uninitialized.
Signed-off-by: Yifeng Sun
---
lib/conntrack.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/conntrack.c b/lib/conntrack.c
index e5266e579452..86c16b2fbe77 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -1138,7 +1138,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
handle_nat(pkt, conn, zone, ctx->reply, ctx->icmp_related);
}
-} else if (check_orig_tuple(ct, pkt, ctx, now, , nat_action_info)) {
+} else if (pkt->md.ct_state
+ && check_orig_tuple(ct, pkt, ctx, now, , nat_action_info))
{
create_new_conn = conn_update_state(ct, pkt, ctx, conn, now);
} else {
if (ctx->icmp_related) {
--
2.7.4
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
