| Message ID | 20240510154554.109617-1-pvalerio@redhat.com |
|---|---|
| State | Accepted |
| Commit | 3833506db0de7a9c7e72b82323bc1c355d2c03b3 |
| Delegated to: | Simon Horman |
| Headers | show |
| Series | [ovs-dev,v2] conntrack: Fully initialize conn struct before insertion. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Fri, May 10, 2024 at 05:45:54PM +0200, Paolo Valerio wrote: > From: Mike Pattrick <mkp@redhat.com> > > 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 <mkp@redhat.com> > Co-authored-by: Paolo Valerio <pvalerio@redhat.com> > Signed-off-by: Paolo Valerio <pvalerio@redhat.com> Acked-by: Simon Horman <horms@ovn.org> (I accidently sent the above for v1 just now)
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 <mkp@redhat.com> >> >> 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 <mkp@redhat.com> >> Co-authored-by: Paolo Valerio <pvalerio@redhat.com> >> Signed-off-by: Paolo Valerio <pvalerio@redhat.com> > > Acked-by: Simon Horman <horms@ovn.org> Thanks, Paolo, Mike and Simon! Applied and backported down to 2.17. Bets regards, Ilya Maximets.
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;