When the selftest 'tap.c' is compiled with '-D_FORTIFY_SOURCE=3', the strcpy() in rtattr_add_strsz() is replaced with a checked version which causes the test to consistently fail when compiled with toolchains for which this option is enabled by default.
TAP version 13 1..3 # Starting 3 tests from 1 test cases. # RUN tap.test_packet_valid_udp_gso ... *** buffer overflow detected ***: terminated # test_packet_valid_udp_gso: Test terminated by assertion # FAIL tap.test_packet_valid_udp_gso not ok 1 tap.test_packet_valid_udp_gso # RUN tap.test_packet_valid_udp_csum ... *** buffer overflow detected ***: terminated # test_packet_valid_udp_csum: Test terminated by assertion # FAIL tap.test_packet_valid_udp_csum not ok 2 tap.test_packet_valid_udp_csum # RUN tap.test_packet_crash_tap_invalid_eth_proto ... *** buffer overflow detected ***: terminated # test_packet_crash_tap_invalid_eth_proto: Test terminated by assertion # FAIL tap.test_packet_crash_tap_invalid_eth_proto not ok 3 tap.test_packet_crash_tap_invalid_eth_proto # FAILED: 0 / 3 tests passed. # Totals: pass:0 fail:3 xfail:0 xpass:0 skip:0 error:0
A buffer overflow is detected by the fortified glibc __strcpy_chk() since the __builtin_object_size() of `RTA_DATA(rta)` is incorrectly reported as 1, even though there is ample space in its bounding buffer `req`.
Using the unchecked function memcpy() here instead allows us to match the way rtattr_add_str() is written while avoiding the spurious test failure.
Fixes: 2e64fe4624d1 ("selftests: add few test cases for tap driver") Signed-off-by: Alice C. Munduruca alice.munduruca@canonical.com --- tools/testing/selftests/net/tap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/net/tap.c b/tools/testing/selftests/net/tap.c index 247c3b3ac1c9..dd961b629295 100644 --- a/tools/testing/selftests/net/tap.c +++ b/tools/testing/selftests/net/tap.c @@ -67,7 +67,7 @@ static struct rtattr *rtattr_add_strsz(struct nlmsghdr *nh, unsigned short type, { struct rtattr *rta = rtattr_add(nh, type, strlen(s) + 1);
- strcpy(RTA_DATA(rta), s); + memcpy(RTA_DATA(rta), s, strlen(s) + 1); return rta; }
Alice C. Munduruca wrote:
When the selftest 'tap.c' is compiled with '-D_FORTIFY_SOURCE=3', the strcpy() in rtattr_add_strsz() is replaced with a checked version which causes the test to consistently fail when compiled with toolchains for which this option is enabled by default.
TAP version 13 1..3 # Starting 3 tests from 1 test cases. # RUN tap.test_packet_valid_udp_gso ... *** buffer overflow detected ***: terminated # test_packet_valid_udp_gso: Test terminated by assertion # FAIL tap.test_packet_valid_udp_gso not ok 1 tap.test_packet_valid_udp_gso # RUN tap.test_packet_valid_udp_csum ... *** buffer overflow detected ***: terminated # test_packet_valid_udp_csum: Test terminated by assertion # FAIL tap.test_packet_valid_udp_csum not ok 2 tap.test_packet_valid_udp_csum # RUN tap.test_packet_crash_tap_invalid_eth_proto ... *** buffer overflow detected ***: terminated # test_packet_crash_tap_invalid_eth_proto: Test terminated by assertion # FAIL tap.test_packet_crash_tap_invalid_eth_proto not ok 3 tap.test_packet_crash_tap_invalid_eth_proto # FAILED: 0 / 3 tests passed. # Totals: pass:0 fail:3 xfail:0 xpass:0 skip:0 error:0
A buffer overflow is detected by the fortified glibc __strcpy_chk() since the __builtin_object_size() of `RTA_DATA(rta)` is incorrectly reported as 1, even though there is ample space in its bounding buffer `req`.
Using the unchecked function memcpy() here instead allows us to match the way rtattr_add_str() is written while avoiding the spurious test failure.
Fixes: 2e64fe4624d1 ("selftests: add few test cases for tap driver") Signed-off-by: Alice C. Munduruca alice.munduruca@canonical.com
tools/testing/selftests/net/tap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/net/tap.c b/tools/testing/selftests/net/tap.c index 247c3b3ac1c9..dd961b629295 100644 --- a/tools/testing/selftests/net/tap.c +++ b/tools/testing/selftests/net/tap.c @@ -67,7 +67,7 @@ static struct rtattr *rtattr_add_strsz(struct nlmsghdr *nh, unsigned short type, { struct rtattr *rta = rtattr_add(nh, type, strlen(s) + 1);
- strcpy(RTA_DATA(rta), s);
- memcpy(RTA_DATA(rta), s, strlen(s) + 1);
Could call strlen(s) only once in the function.
Why does rtattr_add_str do the same without the terminating '\0'? It is only used with IFNAME so assumes max size of IFNAMSIZ perhaps?
return rta; } -- 2.48.1
On Fri Dec 12, 2025 at 2:18 PM EST, Willem de Bruijn wrote:
Alice C. Munduruca wrote:
When the selftest 'tap.c' is compiled with '-D_FORTIFY_SOURCE=3', the strcpy() in rtattr_add_strsz() is replaced with a checked version which causes the test to consistently fail when compiled with toolchains for which this option is enabled by default. >> TAP version 13 1..3 # Starting 3 tests from 1 test cases. # RUN tap.test_packet_valid_udp_gso ... *** buffer overflow detected ***: terminated # test_packet_valid_udp_gso: Test terminated by assertion # FAIL tap.test_packet_valid_udp_gso not ok 1 tap.test_packet_valid_udp_gso # RUN tap.test_packet_valid_udp_csum ... *** buffer overflow detected ***: terminated # test_packet_valid_udp_csum: Test terminated by assertion # FAIL tap.test_packet_valid_udp_csum not ok 2 tap.test_packet_valid_udp_csum # RUN tap.test_packet_crash_tap_invalid_eth_proto ... *** buffer overflow detected ***: terminated # test_packet_crash_tap_invalid_eth_proto: Test terminated by assertion # FAIL tap.test_packet_crash_tap_invalid_eth_proto not ok 3 tap.test_packet_crash_tap_invalid_eth_proto # FAILED: 0 / 3 tests passed. # Totals: pass:0 fail:3 xfail:0 xpass:0 skip:0 error:0
A buffer overflow is detected by the fortified glibc __strcpy_chk() since the __builtin_object_size() of `RTA_DATA(rta)` is incorrectly reported as 1, even though there is ample space in its bounding buffer `req`.
Using the unchecked function memcpy() here instead allows us to match the way rtattr_add_str() is written while avoiding the spurious test failure.
Fixes: 2e64fe4624d1 ("selftests: add few test cases for tap driver") Signed-off-by: Alice C. Munduruca alice.munduruca@canonical.com
tools/testing/selftests/net/tap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/net/tap.c b/tools/testing/selftests/net/tap.c index 247c3b3ac1c9..dd961b629295 100644 --- a/tools/testing/selftests/net/tap.c +++ b/tools/testing/selftests/net/tap.c @@ -67,7 +67,7 @@ static struct rtattr *rtattr_add_strsz(struct nlmsghdr *nh, unsigned short type, { struct rtattr *rta = rtattr_add(nh, type, strlen(s) + 1);
- strcpy(RTA_DATA(rta), s);
- memcpy(RTA_DATA(rta), s, strlen(s) + 1);
Could call strlen(s) only once in the function.
Why does rtattr_add_str do the same without the terminating '\0'? It is only used with IFNAME so assumes max size of IFNAMSIZ perhaps?
I'm not sure why that was the case previously, although this hypothesis seems reasonable. I'll go ahead and fold both of the rtattr_add_str{,sz}() functions into a single one which includes the null-termination, as that seems to be the correct behaviour in this instance. (while addressing the strlen comment above)
I'll also leave some time for comments on the above before submitting the changes in a v3 tomorrow.
return rta; } -- 2.48.1
Thanks for the review, - Alice C. Munduruca
linux-kselftest-mirror@lists.linaro.org
-
Alice C. Munduruca -
Willem de Bruijn