On 3 Mar 2026, at 3:20, Mike Pattrick wrote:
> 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. Thanks for the review, and I agree hashing padding sounds wrong. I’ve copied Aaron and Paolo, who will be looking at the conntrack rework for HW offload. Maybe they can take a look at this while they're at it. //Eelco > Acked-by: Mike Pattrick <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
