[PATCH v3 net] ipvs: fix UB due to uninitialized stack access in ip_vs_protocol_init()

[PATCH v3 net] ipvs: fix UB due to uninitialized stack access in ip_vs_protocol_init()

[Jinghao Jia]

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.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202402100205.PWXIz1ZK-lkp@intel.com/
Co-developed-by: Ruowen Qin <ruqin@redhat.com>
Signed-off-by: Ruowen Qin <ruqin@redhat.com>
Signed-off-by: Jinghao Jia <jinghao7@illinois.edu>
---
Changelog:
v2 -> v3:
v2: https://lore.kernel.org/lkml/20241122045257.27452-1-jinghao7@illinois.edu/
* Fix changelog format based on Julian's feedback

v1 -> v2:
v1: https://lore.kernel.org/lkml/20241111065105.82431-1-jinghao7@illinois.edu/
* 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

 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

Re: [PATCH v3 net] ipvs: fix UB due to uninitialized stack access in ip_vs_protocol_init()

[Julian Anastasov]

	Hello,

On Sat, 23 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.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202402100205.PWXIz1ZK-lkp@intel.com/
> Co-developed-by: Ruowen Qin <ruqin@redhat.com>
> Signed-off-by: Ruowen Qin <ruqin@redhat.com>
> Signed-off-by: Jinghao Jia <jinghao7@illinois.edu>

	Looks good to me, thanks!

Acked-by: Julian Anastasov <ja@ssi.bg>

> ---
> Changelog:
> v2 -> v3:
> v2: https://lore.kernel.org/lkml/20241122045257.27452-1-jinghao7@illinois.edu/
> * Fix changelog format based on Julian's feedback
> 
> v1 -> v2:
> v1: https://lore.kernel.org/lkml/20241111065105.82431-1-jinghao7@illinois.edu/
> * 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
> 
>  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@ssi.bg>

Re: [PATCH v3 net] ipvs: fix UB due to uninitialized stack access in ip_vs_protocol_init()

[Paolo Abeni]

On 11/23/24 10:42, 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.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202402100205.PWXIz1ZK-lkp@intel.com/
> Co-developed-by: Ruowen Qin <ruqin@redhat.com>
> Signed-off-by: Ruowen Qin <ruqin@redhat.com>
> Signed-off-by: Jinghao Jia <jinghao7@illinois.edu>

@Pablo, @Simon, @Julian: recent ipvs patches landed either on the
net(-next) trees or the netfiler trees according to a random (?) pattern.

What is your preference here? Should such patches go via netfilter or
net? Or something else. FTR, I *think* netfilter should be the
preferable target, but I'm open to other options.

Thanks,

Paolo

Re: [PATCH v3 net] ipvs: fix UB due to uninitialized stack access in ip_vs_protocol_init()

[Julian Anastasov]

	Hello,

On Thu, 28 Nov 2024, Paolo Abeni wrote:

> On 11/23/24 10:42, 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.
> > 
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202402100205.PWXIz1ZK-lkp@intel.com/
> > Co-developed-by: Ruowen Qin <ruqin@redhat.com>
> > Signed-off-by: Ruowen Qin <ruqin@redhat.com>
> > Signed-off-by: Jinghao Jia <jinghao7@illinois.edu>
> 
> @Pablo, @Simon, @Julian: recent ipvs patches landed either on the
> net(-next) trees or the netfiler trees according to a random (?) pattern.
> 
> What is your preference here? Should such patches go via netfilter or
> net? Or something else. FTR, I *think* netfilter should be the
> preferable target, but I'm open to other options.

	IPVS patches should go always via Netfilter trees.
It is my fault to tell people to use the 'net' tag, I'll
recommend the proper nf tree the next time. Sorry for the
confusion.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH v3 net] ipvs: fix UB due to uninitialized stack access in ip_vs_protocol_init()

[Pablo Neira Ayuso]

On Thu, Nov 28, 2024 at 01:18:39PM +0200, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Thu, 28 Nov 2024, Paolo Abeni wrote:
> 
> > On 11/23/24 10:42, 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.
> > > 
> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Closes: https://lore.kernel.org/oe-kbuild-all/202402100205.PWXIz1ZK-lkp@intel.com/
> > > Co-developed-by: Ruowen Qin <ruqin@redhat.com>
> > > Signed-off-by: Ruowen Qin <ruqin@redhat.com>
> > > Signed-off-by: Jinghao Jia <jinghao7@illinois.edu>
> > 
> > @Pablo, @Simon, @Julian: recent ipvs patches landed either on the
> > net(-next) trees or the netfiler trees according to a random (?) pattern.
> > 
> > What is your preference here? Should such patches go via netfilter or
> > net? Or something else. FTR, I *think* netfilter should be the
> > preferable target, but I'm open to other options.
> 
> 	IPVS patches should go always via Netfilter trees.
> It is my fault to tell people to use the 'net' tag, I'll
> recommend the proper nf tree the next time. Sorry for the
> confusion.

No issue, I have applied this to nf.git, thanks for the clarification.