Hi Julian,
Thanks for getting back to us!
On 11/18/24 6:41 AM, Julian Anastasov wrote:
>
> Hello,
>
> On Mon, 11 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) to more DCE opportunities
One small request: if you could help us remove the extra "to" in the above
sentence when committing this patch, it would be great.
>> 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.
>>
>> Reported-by: kernel test robot <lkp@xxxxxxxxx>
>> Closes: https://urldefense.com/v3/__https://lore.kernel.org/oe-kbuild-all/202402100205.PWXIz1ZK-lkp@xxxxxxxxx/__;!!DZ3fjg!823fsY09q3IcP8uThu-yUuuQaiwQOR7gZJhV9JNWdxzerlkYJ4JkZGYuq4iO1DKqaErCulk1CGir$
>> 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! I assume it is for
> net-next/nf-next, right?
I am actually not familiar with the netfilter trees. IMHO this should also be
back-ported to the stable kernels -- I wonder if net-next/nf-next is a good
tree for this?
>
> Acked-by: Julian Anastasov <ja@xxxxxx>
>
>> ---
>> 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>
>
Best,
Jinghao
