netdev - Re: [PATCH v2 net] ipvs: fix UB due to uninitialized stack access in ip_vs_protocol

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: <298cf980-b66e-4806-ad6b-8cfff4c3dd81@illinois.edu>
Date: Fri, 22 Nov 2024 12:02:46 -0600
From: Jinghao Jia <jinghao7@...inois.edu>
To: Julian Anastasov <ja@....bg>
Cc: Simon Horman <horms@...ge.net.au>,
        Pablo Neira Ayuso
 <pablo@...filter.org>,
        Jozsef Kadlecsik <kadlec@...filter.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Bill Wendling
 <morbo@...gle.com>,
        Justin Stitt <justinstitt@...gle.com>, Kees Cook <kees@...nel.org>,
        netdev@...r.kernel.org, lvs-devel@...r.kernel.org,
        netfilter-devel@...r.kernel.org, coreteam@...filter.org,
        linux-kernel@...r.kernel.org, llvm@...ts.linux.dev,
        kernel test robot <lkp@...el.com>, Ruowen Qin <ruqin@...hat.com>
Subject: Re: [PATCH v2 net] ipvs: fix UB due to uninitialized stack access in
 ip_vs_protocol_init()

Hi Julian,

On 11/22/24 5:43 AM, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Thu, 21 Nov 2024, Jinghao Jia wrote:
> 
>> Under certain kernel configurations when building with Clang/LLVM, the
>> compiler does not generate a return or jump as the terminator
>> instruction for ip_vs_protocol_init(), triggering the following objtool
>> warning during build time:
>>
>>   vmlinux.o: warning: objtool: ip_vs_protocol_init() falls through to next function __initstub__kmod_ip_vs_rr__935_123_ip_vs_rr_init6()
>>
>> At runtime, this either causes an oops when trying to load the ipvs
>> module or a boot-time panic if ipvs is built-in. This same issue has
>> been reported by the Intel kernel test robot previously.
>>
>> Digging deeper into both LLVM and the kernel code reveals this to be a
>> undefined behavior problem. ip_vs_protocol_init() uses a on-stack buffer
>> of 64 chars to store the registered protocol names and leaves it
>> uninitialized after definition. The function calls strnlen() when
>> concatenating protocol names into the buffer. With CONFIG_FORTIFY_SOURCE
>> strnlen() performs an extra step to check whether the last byte of the
>> input char buffer is a null character (commit 3009f891bb9f ("fortify:
>> Allow strlen() and strnlen() to pass compile-time known lengths")).
>> This, together with possibly other configurations, cause the following
>> IR to be generated:
>>
>>   define hidden i32 @ip_vs_protocol_init() local_unnamed_addr #5 section ".init.text" align 16 !kcfi_type !29 {
>>     %1 = alloca [64 x i8], align 16
>>     ...
>>
>>   14:                                               ; preds = %11
>>     %15 = getelementptr inbounds i8, ptr %1, i64 63
>>     %16 = load i8, ptr %15, align 1
>>     %17 = tail call i1 @llvm.is.constant.i8(i8 %16)
>>     %18 = icmp eq i8 %16, 0
>>     %19 = select i1 %17, i1 %18, i1 false
>>     br i1 %19, label %20, label %23
>>
>>   20:                                               ; preds = %14
>>     %21 = call i64 @strlen(ptr noundef nonnull dereferenceable(1) %1) #23
>>     ...
>>
>>   23:                                               ; preds = %14, %11, %20
>>     %24 = call i64 @strnlen(ptr noundef nonnull dereferenceable(1) %1, i64 noundef 64) #24
>>     ...
>>   }
>>
>> The above code calculates the address of the last char in the buffer
>> (value %15) and then loads from it (value %16). Because the buffer is
>> never initialized, the LLVM GVN pass marks value %16 as undefined:
>>
>>   %13 = getelementptr inbounds i8, ptr %1, i64 63
>>   br i1 undef, label %14, label %17
>>
>> This gives later passes (SCCP, in particular) more DCE opportunities by
>> propagating the undef value further, and eventually removes everything
>> after the load on the uninitialized stack location:
>>
>>   define hidden i32 @ip_vs_protocol_init() local_unnamed_addr #0 section ".init.text" align 16 !kcfi_type !11 {
>>     %1 = alloca [64 x i8], align 16
>>     ...
>>
>>   12:                                               ; preds = %11
>>     %13 = getelementptr inbounds i8, ptr %1, i64 63
>>     unreachable
>>   }
>>
>> In this way, the generated native code will just fall through to the
>> next function, as LLVM does not generate any code for the unreachable IR
>> instruction and leaves the function without a terminator.
>>
>> Zero the on-stack buffer to avoid this possible UB.
>>
>> Changelog:
>> ---
> 
> 	You can not add --- before the following headers.
> 'git am file.patch' can show the result of applying the patch.

Oops my bad -- have never done a patch with changelogs inside the commit
message.

> 
>> v1 -> v2:
>> v1: https://urldefense.com/v3/__https://lore.kernel.org/lkml/20241111065105.82431-1-jinghao7@illinois.edu/__;!!DZ3fjg!45RxVgl4P0t8X0Vx6JFjX8J0xtWPfXijFBJTXvQ6B89xyEQqWT53QYyG9Ztlo3lfyadT2FdfZexm$ 
>> * Fix small error in commit message
>> * Address Julian's feedback:
>>   * Make this patch target the net tree rather than net-next
>>   * Add a "Fixes" tag for the initial git commit
>>
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Reported-by: kernel test robot <lkp@...el.com>
>> Closes: https://urldefense.com/v3/__https://lore.kernel.org/oe-kbuild-all/202402100205.PWXIz1ZK-lkp@intel.com/__;!!DZ3fjg!45RxVgl4P0t8X0Vx6JFjX8J0xtWPfXijFBJTXvQ6B89xyEQqWT53QYyG9Ztlo3lfyadT2Ko7xLwQ$ 
>> Co-developed-by: Ruowen Qin <ruqin@...hat.com>
>> Signed-off-by: Ruowen Qin <ruqin@...hat.com>
>> Signed-off-by: Jinghao Jia <jinghao7@...inois.edu>
>> ---
> 
> 	Please send v3, you can move your changelog here,
> after the above ---

Sounds good! I will roll out a v3 later today.

> 
>>  net/netfilter/ipvs/ip_vs_proto.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/net/netfilter/ipvs/ip_vs_proto.c b/net/netfilter/ipvs/ip_vs_proto.c
>> index f100da4ba3bc..a9fd1d3fc2cb 100644
>> --- a/net/netfilter/ipvs/ip_vs_proto.c
>> +++ b/net/netfilter/ipvs/ip_vs_proto.c
>> @@ -340,7 +340,7 @@ void __net_exit ip_vs_protocol_net_cleanup(struct netns_ipvs *ipvs)
>>  
>>  int __init ip_vs_protocol_init(void)
>>  {
>> -	char protocols[64];
>> +	char protocols[64] = { 0 };
>>  #define REGISTER_PROTOCOL(p)			\
>>  	do {					\
>>  		register_ip_vs_protocol(p);	\
>> @@ -348,8 +348,6 @@ int __init ip_vs_protocol_init(void)
>>  		strcat(protocols, (p)->name);	\
>>  	} while (0)
>>  
>> -	protocols[0] = '\0';
>> -	protocols[2] = '\0';
>>  #ifdef CONFIG_IP_VS_PROTO_TCP
>>  	REGISTER_PROTOCOL(&ip_vs_protocol_tcp);
>>  #endif
>> -- 
>> 2.47.0
> 
> Regards
> 
> --
> Julian Anastasov <ja@....bg>
> 

Best,
Jinghao