Re: [ovs-dev] [PATCH v2] conntrack: Fully initialize conn struct before insertion.
On 5/13/24 14:38, Simon Horman wrote:
> On Fri, May 10, 2024 at 05:45:54PM +0200, Paolo Valerio wrote:
>> From: Mike Pattrick
>>
>> In case packets are concurrently received in both directions, there's
>> a chance that the ones in the reverse direction get received right
>> after the connection gets added to the connection tracker but before
>> some of the connection's fields are fully initialized.
>> This could cause OVS to access potentially invalid, as the lookup may
>> end up retrieving the wrong offsets during CONTAINER_OF(), or
>> uninitialized memory.
>>
>> This may happen in case of regular NAT or all-zero SNAT.
>>
>> Fix it by initializing early the connections fields.
>>
>> Fixes: 1116459b3ba8 ("conntrack: Remove nat_conn introducing key
>> directionality.")
>> Reported-at: https://issues.redhat.com/browse/FDP-616
>> Signed-off-by: Mike Pattrick
>> Co-authored-by: Paolo Valerio
>> Signed-off-by: Paolo Valerio
>
> Acked-by: Simon Horman
Thanks, Paolo, Mike and Simon!
Applied and backported down to 2.17.
Bets regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] conntrack: Fully initialize conn struct before insertion.
On Fri, May 10, 2024 at 05:45:54PM +0200, Paolo Valerio wrote:
> From: Mike Pattrick
>
> In case packets are concurrently received in both directions, there's
> a chance that the ones in the reverse direction get received right
> after the connection gets added to the connection tracker but before
> some of the connection's fields are fully initialized.
> This could cause OVS to access potentially invalid, as the lookup may
> end up retrieving the wrong offsets during CONTAINER_OF(), or
> uninitialized memory.
>
> This may happen in case of regular NAT or all-zero SNAT.
>
> Fix it by initializing early the connections fields.
>
> Fixes: 1116459b3ba8 ("conntrack: Remove nat_conn introducing key
> directionality.")
> Reported-at: https://issues.redhat.com/browse/FDP-616
> Signed-off-by: Mike Pattrick
> Co-authored-by: Paolo Valerio
> Signed-off-by: Paolo Valerio
Acked-by: Simon Horman
(I accidently sent the above for v1 just now)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] conntrack: Fully initialize conn struct before insertion.
From: Mike Pattrick
In case packets are concurrently received in both directions, there's
a chance that the ones in the reverse direction get received right
after the connection gets added to the connection tracker but before
some of the connection's fields are fully initialized.
This could cause OVS to access potentially invalid, as the lookup may
end up retrieving the wrong offsets during CONTAINER_OF(), or
uninitialized memory.
This may happen in case of regular NAT or all-zero SNAT.
Fix it by initializing early the connections fields.
Fixes: 1116459b3ba8 ("conntrack: Remove nat_conn introducing key
directionality.")
Reported-at: https://issues.redhat.com/browse/FDP-616
Signed-off-by: Mike Pattrick
Co-authored-by: Paolo Valerio
Signed-off-by: Paolo Valerio
---
lib/conntrack.c | 24 +++-
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 16e1c8bb5..5fdfe98de 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -947,6 +947,18 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
nc->parent_key = alg_exp->parent_key;
}
+ovs_mutex_init_adaptive(&nc->lock);
+atomic_flag_clear(&nc->reclaimed);
+fwd_key_node->dir = CT_DIR_FWD;
+rev_key_node->dir = CT_DIR_REV;
+
+if (zl) {
+nc->admit_zone = zl->czl.zone;
+nc->zone_limit_seq = zl->czl.zone_limit_seq;
+} else {
+nc->admit_zone = INVALID_ZONE;
+}
+
if (nat_action_info) {
nc->nat_action = nat_action_info->nat_action;
@@ -972,22 +984,16 @@ conn_not_found(struct conntrack *ct, struct dp_packet
*pkt,
&rev_key_node->cm_node, rev_hash);
}
-ovs_mutex_init_adaptive(&nc->lock);
-atomic_flag_clear(&nc->reclaimed);
-fwd_key_node->dir = CT_DIR_FWD;
-rev_key_node->dir = CT_DIR_REV;
cmap_insert(&ct->conns[ctx->key.zone],
&fwd_key_node->cm_node, ctx->hash);
conn_expire_push_front(ct, nc);
atomic_count_inc(&ct->n_conn);
-ctx->conn = nc; /* For completeness. */
+
if (zl) {
-nc->admit_zone = zl->czl.zone;
-nc->zone_limit_seq = zl->czl.zone_limit_seq;
atomic_count_inc(&zl->czl.count);
-} else {
-nc->admit_zone = INVALID_ZONE;
}
+
+ctx->conn = nc; /* For completeness. */
}
return nc;
--
2.45.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
