Bug 265664 - Undefined behaviour in sys/netinet/tcp_lro.h
Summary: Undefined behaviour in sys/netinet/tcp_lro.h
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Mark Linimon
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-08-05 18:38 UTC by Nick Reilly
Modified: 2023-08-01 18:33 UTC (History)
4 users (show)

See Also:

Attachments
Does this patch work for you? (1.61 KB, patch)
2022-11-28 22:58 UTC, Hans Petter Selasky 		no flags Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Reilly 2022-08-05 18:38:04 UTC 
union lro_address {
	u_long raw[1];
	struct {
		uint8_t lro_type;	/* internal */
...
	};
} __aligned(sizeof(u_long));

#define	LRO_RAW_ADDRESS_MAX \
    (sizeof(union lro_address) / sizeof(u_long))

There are then a number of functions both in tcp_lro.h and tcp_lro.c that do things like:

for (unsigned i = 0; i < LRO_RAW_ADDRESS_MAX; i++) {
		if (pa->raw[i] != pb->raw[i])

This is undefined behaviour.

Either the member "raw" should be removed and the iteration over the structure rewritten, or the definition should be improved so that the array size actually encompasses the entire struct.
Comment 1 Hans Petter Selasky freebsd_committer freebsd_triage 2022-11-28 22:58:00 UTC 
Created attachment 238409 [details]
Does this patch work for you?

Not extensivly compile tested, but will do that before submitting it.
Comment 2 Hans Petter Selasky freebsd_committer freebsd_triage 2022-11-28 22:58:34 UTC 
rrs@ please have a look at the attached patch!
Comment 3 Nick Reilly 2022-12-08 17:33:29 UTC 
Yes, this patch works for me.
Comment 4 Ed Maste freebsd_committer freebsd_triage 2023-05-11 23:27:10 UTC 
@hps will you pick this up again?
Comment 5 Mark Linimon freebsd_committer freebsd_triage 2023-07-25 19:39:01 UTC 
To emaste: was this problem ever handled?
Comment 6 Ed Maste freebsd_committer freebsd_triage 2023-07-25 19:51:51 UTC 
It looks like this was in fact committed (but was not MFC'd)

commit e0d8add4af0be1d37ede9a16f46424dc08f0d95e
Author: Hans Petter Selasky <hselasky@FreeBSD.org>
Date:   Mon Nov 28 23:56:16 2022 +0100

    tcp_lro: Fix for undefined behaviour.
    
    Make sure the size of the raw[] array in the lro_address union is
    correctly set at compile time, so that static code analysis tools
    do not report undefined behaviour.
    
    MFC after:      1 week
    Sponsored by:   NVIDIA Networking
Comment 7 commit-hook freebsd_committer freebsd_triage 2023-08-01 18:30:01 UTC 
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=bc50720b321667c71d77d3f0c692a59c77f955da

commit bc50720b321667c71d77d3f0c692a59c77f955da
Author:     Hans Petter Selasky <hselasky@FreeBSD.org>
AuthorDate: 2022-11-28 22:56:16 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2023-08-01 17:13:06 +0000

    tcp_lro: Fix for undefined behaviour.

    Make sure the size of the raw[] array in the lro_address union is
    correctly set at compile time, so that static code analysis tools
    do not report undefined behaviour.

    PR:             265664
    Sponsored by:   NVIDIA Networking

    (cherry picked from commit e0d8add4af0be1d37ede9a16f46424dc08f0d95e)

 sys/netinet/tcp_lro.h | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
Comment 8 Ed Maste freebsd_committer freebsd_triage 2023-08-01 18:33:18 UTC 
I merged the patch to stable/13 now.

Rest in peace, Hans Petter.