|
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@xxxxxxxxx>
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202402100205.PWXIz1ZK-lkp@xxxxxxxxx/
> Co-developed-by: Ruowen Qin <ruqin@xxxxxxxxxx>
> Signed-off-by: Ruowen Qin <ruqin@xxxxxxxxxx>
> Signed-off-by: Jinghao Jia <jinghao7@xxxxxxxxxxxx>
Looks good to me, thanks!
Acked-by: Julian Anastasov <ja@xxxxxx>
> ---
> Changelog:
> v2 -> v3:
> v2: https://lore.kernel.org/lkml/20241122045257.27452-1-jinghao7@xxxxxxxxxxxx/
> * Fix changelog format based on Julian's feedback
>
> v1 -> v2:
> v1: https://lore.kernel.org/lkml/20241111065105.82431-1-jinghao7@xxxxxxxxxxxx/
> * 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@xxxxxx>
|
