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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <DEYXG6ZXHLE0.39WQE3DELH3QQ@canonical.com>
Date: Mon, 15 Dec 2025 11:22:52 -0500
From: "Alice C. Munduruca" <alice.munduruca@...onical.com>
To: "Willem de Bruijn" <willemdebruijn.kernel@...il.com>,
 <netdev@...r.kernel.org>
Cc: <linux-kselftest@...r.kernel.org>, "David S. Miller"
 <davem@...emloft.net>, "Eric Dumazet" <edumazet@...gle.com>, "Jakub
 Kicinski" <kuba@...nel.org>, "Paolo Abeni" <pabeni@...hat.com>, "Simon
 Horman" <horms@...nel.org>, "Shuah Khan" <shuah@...nel.org>, "Willem de
 Bruijn" <willemb@...gle.com>
Subject: Re: [PATCH net v2] selftests: net: fix "buffer overflow detected"
 for tap.c

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@...onical.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