On Mon, Mar 2, 2026 at 7:22 AM Eelco Chaudron <[email protected]> wrote:
> Coverity reports an out-of-bounds read warning (CID 278418) in the
> conn_key_hash() function when using the expression '(&key->dst + 1)'
> to calculate the start address for hashing the remaining fields of
> the conn_key structure.
>
> While the original pointer arithmetic is functionally correct, the
> expression '&key->dst + 1' is flagged by static analysis as it appears
> to access beyond the bounds of the 'dst' field array.
>
> Fix this by using explicit byte-level pointer arithmetic with
> offsetof() and sizeof operators instead of struct pointer increment.
> This makes the code clearer to static analyzers while maintaining
> the same functionality.
>
> Fixes: a489b16854b5 ("conntrack: New userspace connection tracker.")
> Signed-off-by: Eelco Chaudron <[email protected]>
> ---
> lib/conntrack.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 00262a0c6..921f63cfe 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -2317,6 +2317,9 @@ static uint32_t
> conn_key_hash(const struct conn_key *key, uint32_t basis)
> {
> uint32_t hsrc, hdst, hash;
> + const uint32_t *start;
> + const uint32_t *end;
> +
> hsrc = hdst = basis;
> hsrc = ct_endpoint_hash_add(hsrc, &key->src);
> hdst = ct_endpoint_hash_add(hdst, &key->dst);
> @@ -2325,9 +2328,11 @@ conn_key_hash(const struct conn_key *key, uint32_t
> basis)
> hash = hsrc ^ hdst;
>
> /* Hash the rest of the key(L3 and L4 types and zone). */
> - return hash_words((uint32_t *) (&key->dst + 1),
> - (uint32_t *) (key + 1) - (uint32_t *) (&key->dst +
> 1),
> - hash);
> + start = ALIGNED_CAST(const uint32_t *,
> + (const char *) key + offsetof(struct conn_key,
> dst)
> + + sizeof key->dst);
> + end = ALIGNED_CAST(const uint32_t *, key + 1);
> + return hash_words(start, end - start, hash);
>
This does maintain identical behaviour. But looking closer, it looks
strange to hash implicit padding bytes like that. I guess those pad bytes
area always initialized to zero, but it seems tenuous.
I guess it's fine for now, we may want to review this later.
Acked-by: Mike Pattrick <[email protected]>
> }
>
> static void
> --
> 2.52.0
>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
