linux-kernel - Re: [PATCH v3 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: <Z0heLGWuOEkC2n35@calendula>
Date: Thu, 28 Nov 2024 13:12:28 +0100
From: Pablo Neira Ayuso <pablo@...filter.org>
To: Julian Anastasov <ja@....bg>
Cc: Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...ge.net.au>,
	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,
	"David S. Miller" <davem@...emloft.net>,
	kernel test robot <lkp@...el.com>, Ruowen Qin <ruqin@...hat.com>,
	Jinghao Jia <jinghao7@...inois.edu>,
	Jozsef Kadlecsik <kadlec@...filter.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Kees Cook <kees@...nel.org>,
	Nathan Chancellor <nathan@...nel.org>,
	Nick Desaulniers <ndesaulniers@...gle.com>,
	Bill Wendling <morbo@...gle.com>,
	Justin Stitt <justinstitt@...gle.com>
Subject: Re: [PATCH v3 net] ipvs: fix UB due to uninitialized stack access in
 ip_vs_protocol_init()

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@...el.com>
> > > Closes: https://lore.kernel.org/oe-kbuild-all/202402100205.PWXIz1ZK-lkp@intel.com/
> > > Co-developed-by: Ruowen Qin <ruqin@...hat.com>
> > > Signed-off-by: Ruowen Qin <ruqin@...hat.com>
> > > Signed-off-by: Jinghao Jia <jinghao7@...inois.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.