Re: [ovs-dev] [patch v2] conntrack: Fix possible uninitialized memory.
On 2/4/19, 8:53 AM, "ovs-dev-boun...@openvswitch.org on behalf of Aaron
Conole" wrote:
Darrell Ball writes:
> There are a few cases where padding may be undefined according to
> the C standard. Practically, it seems implementations don't have issue,
> but it is better to be safe. The code paths modified are not hot ones.
> Found by inspection.
Better to be safe how? What do implementations not have issue with?
I'm having trouble understanding this commit message as a rationale for
the change.
The conn keys are used in a hash calculation
> Signed-off-by: Darrell Ball
> ---
>
> v2: Found another one; will double check for others later.
Were there actual problems observed somewhere? From what I can see,
none of these accesses did any kind of bit-wise structure memcmp().
Rather, I see use of named fields when doing comparisons. This tells me
that the pad bits should be irrelevant. Probably I missed something.
OTOH, I guess you might also be interested in fixing:
'struct ct_addr first_addr = ct_addr;' on line 2181 of conntrack.c
Existing structure copy should work
and the 'process_ftp_ctl_v6' function. Those do possibly use memcmp()
with indeterminate pad bits, if I'm reading them correctly (but just
cursory reading).
Existing structure copy and the memset zero should work.
> lib/conntrack.c | 13 +
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index e1f4041..e7033a8 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -747,6 +747,7 @@ static struct conn *
> conn_lookup(struct conntrack *ct, const struct conn_key *key, long long
now)
> {
> struct conn_lookup_ctx ctx;
> +memset(, 0, sizeof ctx);
> ctx.conn = NULL;
> ctx.key = *key;
> ctx.hash = conn_key_hash(key, ct->hash_basis);
How does this help? Looking at conn_key_hash it seems to only use those
defined sections of the struct (ie: it doesn't walk the raw memory, it
pulls the fields it needs by assignment). conn_key_cmp also compares
named fields.
Again, For the hash; conn_key has padding
> @@ -896,6 +897,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet
*pkt,
>
> if (nat_action_info) {
> nc->nat_info = xmemdup(nat_action_info, sizeof
*nc->nat_info);
> +memset(conn_for_un_nat_copy, 0, sizeof
*conn_for_un_nat_copy);
Was there an observable bug related to the pad bits here?
No, it is just adhering to C standard
Implementations are 'very unlikely' to have issue.
I didn't walk this path deep enough, but in conntrack.c I didn't see any
related
memcmp() calls.
Again, for the hash later
> if (alg_exp) {
> if (alg_exp->nat_rpl_dst) {
> @@ -934,8 +936,6 @@ conn_not_found(struct conntrack *ct, struct dp_packet
*pkt,
> ct_rwlock_unlock(>resources_lock);
> }
> conn_for_un_nat_copy->conn_type = CT_CONN_TYPE_UN_NAT;
> -conn_for_un_nat_copy->nat_info = NULL;
> -conn_for_un_nat_copy->alg = NULL;
If this was needed here before, isn't it still needed now? There are a
interrim assignments from 'nc', and I think that won't be changed by
doing a memset() (ie: does this still cause a double free if you drop it?).
Not related to double free
Oops, this is still needed in present code base
> nat_packet(pkt, nc, ctx->icmp_related);
> }
> hmap_insert(>buckets[bucket].connections, >node,
ctx->hash);
> @@ -1262,6 +1262,7 @@ process_one(struct conntrack *ct, struct dp_packet
*pkt,
> ct->hash_basis,
> alg_src_ip_wc(ct_alg_ctl));
> if (alg_exp) {
> +memset(_exp_entry, 0, sizeof alg_exp_entry);
> alg_exp_entry = *alg_exp;
> alg_exp = _exp_entry;
> }
> @@ -2024,7 +2025,9 @@ conn_key_hash(const struct conn_key *key, uint32_t
basis)
> static void
> conn_key_reverse(struct conn_key *key)
> {
> -struct ct_endpoint tmp = key->src;
> +struct ct_endpoint tmp;
> +memset(, 0, sizeof tmp);
> +tmp = key->src;
I don't understand the reason to clear the pad bits of tmp here - you
only use the named fields and they should be of the same size (and if
not, the compiler should be performing integer widening / truncation
properly). Did I miss something?
This change was previously removed in V3.
> key->src = key->dst;
> key->dst = tmp;
> }
> @@ -2614,7 +2617,9 @@ static struct alg_exp_node *
>
Re: [ovs-dev] [patch v2] conntrack: Fix possible uninitialized memory.
Darrell Ball writes:
> There are a few cases where padding may be undefined according to
> the C standard. Practically, it seems implementations don't have issue,
> but it is better to be safe. The code paths modified are not hot ones.
> Found by inspection.
Better to be safe how? What do implementations not have issue with?
I'm having trouble understanding this commit message as a rationale for
the change.
> Signed-off-by: Darrell Ball
> ---
>
> v2: Found another one; will double check for others later.
Were there actual problems observed somewhere? From what I can see,
none of these accesses did any kind of bit-wise structure memcmp().
Rather, I see use of named fields when doing comparisons. This tells me
that the pad bits should be irrelevant. Probably I missed something.
OTOH, I guess you might also be interested in fixing:
'struct ct_addr first_addr = ct_addr;' on line 2181 of conntrack.c
and the 'process_ftp_ctl_v6' function. Those do possibly use memcmp()
with indeterminate pad bits, if I'm reading them correctly (but just
cursory reading).
> lib/conntrack.c | 13 +
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index e1f4041..e7033a8 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -747,6 +747,7 @@ static struct conn *
> conn_lookup(struct conntrack *ct, const struct conn_key *key, long long now)
> {
> struct conn_lookup_ctx ctx;
> +memset(, 0, sizeof ctx);
> ctx.conn = NULL;
> ctx.key = *key;
> ctx.hash = conn_key_hash(key, ct->hash_basis);
How does this help? Looking at conn_key_hash it seems to only use those
defined sections of the struct (ie: it doesn't walk the raw memory, it
pulls the fields it needs by assignment). conn_key_cmp also compares
named fields.
> @@ -896,6 +897,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet
> *pkt,
>
> if (nat_action_info) {
> nc->nat_info = xmemdup(nat_action_info, sizeof *nc->nat_info);
> +memset(conn_for_un_nat_copy, 0, sizeof *conn_for_un_nat_copy);
Was there an observable bug related to the pad bits here? I didn't walk
this path deep enough, but in conntrack.c I didn't see any related
memcmp() calls.
> if (alg_exp) {
> if (alg_exp->nat_rpl_dst) {
> @@ -934,8 +936,6 @@ conn_not_found(struct conntrack *ct, struct dp_packet
> *pkt,
> ct_rwlock_unlock(>resources_lock);
> }
> conn_for_un_nat_copy->conn_type = CT_CONN_TYPE_UN_NAT;
> -conn_for_un_nat_copy->nat_info = NULL;
> -conn_for_un_nat_copy->alg = NULL;
If this was needed here before, isn't it still needed now? There are a
interrim assignments from 'nc', and I think that won't be changed by
doing a memset() (ie: does this still cause a double free if you drop it?).
> nat_packet(pkt, nc, ctx->icmp_related);
> }
> hmap_insert(>buckets[bucket].connections, >node, ctx->hash);
> @@ -1262,6 +1262,7 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
> ct->hash_basis,
> alg_src_ip_wc(ct_alg_ctl));
> if (alg_exp) {
> +memset(_exp_entry, 0, sizeof alg_exp_entry);
> alg_exp_entry = *alg_exp;
> alg_exp = _exp_entry;
> }
> @@ -2024,7 +2025,9 @@ conn_key_hash(const struct conn_key *key, uint32_t
> basis)
> static void
> conn_key_reverse(struct conn_key *key)
> {
> -struct ct_endpoint tmp = key->src;
> +struct ct_endpoint tmp;
> +memset(, 0, sizeof tmp);
> +tmp = key->src;
I don't understand the reason to clear the pad bits of tmp here - you
only use the named fields and they should be of the same size (and if
not, the compiler should be performing integer widening / truncation
properly). Did I miss something?
> key->src = key->dst;
> key->dst = tmp;
> }
> @@ -2614,7 +2617,9 @@ static struct alg_exp_node *
> expectation_lookup(struct hmap *alg_expectations, const struct conn_key *key,
> uint32_t basis, bool src_ip_wc)
> {
> -struct conn_key check_key = *key;
> +struct conn_key check_key;
> +memset(_key, 0, sizeof check_key);
> +check_key = *key;
> check_key.src.port = ALG_WC_SRC_PORT;
>
> if (src_ip_wc) {
Again, conn_key_hash and conn_key_cmp should be correct, no?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [patch v2] conntrack: Fix possible uninitialized memory.
There are a few cases where padding may be undefined according to
the C standard. Practically, it seems implementations don't have issue,
but it is better to be safe. The code paths modified are not hot ones.
Found by inspection.
Signed-off-by: Darrell Ball
---
v2: Found another one; will double check for others later.
lib/conntrack.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/lib/conntrack.c b/lib/conntrack.c
index e1f4041..e7033a8 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -747,6 +747,7 @@ static struct conn *
conn_lookup(struct conntrack *ct, const struct conn_key *key, long long now)
{
struct conn_lookup_ctx ctx;
+memset(, 0, sizeof ctx);
ctx.conn = NULL;
ctx.key = *key;
ctx.hash = conn_key_hash(key, ct->hash_basis);
@@ -896,6 +897,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
if (nat_action_info) {
nc->nat_info = xmemdup(nat_action_info, sizeof *nc->nat_info);
+memset(conn_for_un_nat_copy, 0, sizeof *conn_for_un_nat_copy);
if (alg_exp) {
if (alg_exp->nat_rpl_dst) {
@@ -934,8 +936,6 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
ct_rwlock_unlock(>resources_lock);
}
conn_for_un_nat_copy->conn_type = CT_CONN_TYPE_UN_NAT;
-conn_for_un_nat_copy->nat_info = NULL;
-conn_for_un_nat_copy->alg = NULL;
nat_packet(pkt, nc, ctx->icmp_related);
}
hmap_insert(>buckets[bucket].connections, >node, ctx->hash);
@@ -1262,6 +1262,7 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
ct->hash_basis,
alg_src_ip_wc(ct_alg_ctl));
if (alg_exp) {
+memset(_exp_entry, 0, sizeof alg_exp_entry);
alg_exp_entry = *alg_exp;
alg_exp = _exp_entry;
}
@@ -2024,7 +2025,9 @@ conn_key_hash(const struct conn_key *key, uint32_t basis)
static void
conn_key_reverse(struct conn_key *key)
{
-struct ct_endpoint tmp = key->src;
+struct ct_endpoint tmp;
+memset(, 0, sizeof tmp);
+tmp = key->src;
key->src = key->dst;
key->dst = tmp;
}
@@ -2614,7 +2617,9 @@ static struct alg_exp_node *
expectation_lookup(struct hmap *alg_expectations, const struct conn_key *key,
uint32_t basis, bool src_ip_wc)
{
-struct conn_key check_key = *key;
+struct conn_key check_key;
+memset(_key, 0, sizeof check_key);
+check_key = *key;
check_key.src.port = ALG_WC_SRC_PORT;
if (src_ip_wc) {
--
1.9.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
