[PATCH net v2] selftests: net: fix "buffer overflow detected" for tap.c

[PATCH net v2] selftests: net: fix "buffer overflow detected" for tap.c

[Alice C. Munduruca]

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;
 }
 
-- 
2.48.1

Re: [PATCH net v2] selftests: net: fix "buffer overflow detected" for tap.c

[Willem de Bruijn]

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
> 

Re: [PATCH net v2] selftests: net: fix "buffer overflow detected" for tap.c

[Alice C. Munduruca]

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