Re: [PATCH v2 1/2] modutils: check ELF header before calling finit_module()
On Tue, 5 Jan 2021 20:21:12 +0100 Alex Samorukov wrote: > On Tue, 5 Jan 2021 10:47:25 +0800 > > Kang-Che Sung wrote: > > > >> I don't see why the > >> "Invalid ELF header" message would bother you so much, since you > >> won't load kernel modules often. > > As mentioned in my original post, it's a pr_err. It shows up on console! > I am very likely missing something, but why we are trying to load > non-elf modules? We're trying to load compressed ELF modules. Pre-v5.9-rc4 kernels silently told busybox "I can't open this, send uncompressed". Since that version, the kernel complains loudly. Hence the issue. (re-adding busybox list) - Lauri ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2 1/2] modutils: check ELF header before calling finit_module()
On 2021/1/5 上午10:47, Kang-Che Sung wrote: On Mon, Jan 4, 2021 at 6:32 PM Qu Wenruo wrote: On 2021/1/4 下午6:01, Kang-Che Sung wrote: On Sun, Jan 3, 2021 at 12:11 PM Qu Wenruo wrote: finit_module() and init_module() system calls have clear specification to only accept valid ELF image. Although we try finit_module() on compressed modules to let the kernel determine if it's an ELF image, but it's not ideal, especially when newer kernel will complain when some invalid files/memory is passed in. Treat the kernel better by just doing a very basic ELF header check before calling finit_module(). Signed-off-by: Qu Wenruo What is the reason for not letting the kernel do all the ELF sanity checks? Performance? Security? For now this looks like extra code to busybox without obvious benefits. To avoid possible "Invalid ELF header" error message from kernel. Since those system calls are only to accept ELF header, kernel has its right to info the caller that it got some invalid ELF header (even if it's just compressed file). Or did you mean, busybox pursues size so much that it doesn't matter to not follow system call spec? It is normal for the kernel to receive a malformed ELF file through init_module() and it's the kernel's job to reject it. I don't see why the "Invalid ELF header" message would bother you so much, since you won't load kernel modules often. It's true until when some users checks the dmesg and see the error message. I'm personally fine with that temporary error message, but not sure how others end users will feel. Especially considering busybox is used in some distros' initramfs, like Arch, to load initial kernel modules for rootfs/lvm/... Without the context, someone would spend tons of time to debug and finally complain. By "security" I mean, if the kernel would accept any header other than ELF and you want to ensure only ELF is passed to the system call, then it's fine to add that sanity check. Otherwise, there's no benefit for repeating what the kernel would do in busybox. So reducing confusion is never a thing to consider in busybox? Only reducing code size is? Thanks, Qu ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2 1/2] modutils: check ELF header before calling finit_module()
On Tue, 5 Jan 2021 10:47:25 +0800 Kang-Che Sung wrote: > I don't see why the > "Invalid ELF header" message would bother you so much, since you > won't load kernel modules often. As mentioned in my original post, it's a pr_err. It shows up on console! So every boot there's 20 scary errors interspersed with boot messages. No need for the curious to see if dmesg was spammed, it's in their face. - Lauri ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2 1/2] modutils: check ELF header before calling finit_module()
On Mon, Jan 4, 2021 at 6:32 PM Qu Wenruo wrote: > > On 2021/1/4 下午6:01, Kang-Che Sung wrote: > > On Sun, Jan 3, 2021 at 12:11 PM Qu Wenruo wrote: > >> > >> finit_module() and init_module() system calls have clear specification > >> to only accept valid ELF image. > >> > >> Although we try finit_module() on compressed modules to let the kernel > >> determine if it's an ELF image, but it's not ideal, especially when > >> newer kernel will complain when some invalid files/memory is passed in. > >> > >> Treat the kernel better by just doing a very basic ELF header check > >> before calling finit_module(). > >> > >> Signed-off-by: Qu Wenruo > > > > What is the reason for not letting the kernel do all the ELF sanity checks? > > Performance? Security? For now this looks like extra code to busybox > > without obvious benefits. > > > To avoid possible "Invalid ELF header" error message from kernel. > > Since those system calls are only to accept ELF header, kernel has its > right to info the caller that it got some invalid ELF header (even if > it's just compressed file). > > Or did you mean, busybox pursues size so much that it doesn't matter to > not follow system call spec? It is normal for the kernel to receive a malformed ELF file through init_module() and it's the kernel's job to reject it. I don't see why the "Invalid ELF header" message would bother you so much, since you won't load kernel modules often. By "security" I mean, if the kernel would accept any header other than ELF and you want to ensure only ELF is passed to the system call, then it's fine to add that sanity check. Otherwise, there's no benefit for repeating what the kernel would do in busybox. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2 1/2] modutils: check ELF header before calling finit_module()
On 2021/1/4 下午6:01, Kang-Che Sung wrote: On Sun, Jan 3, 2021 at 12:11 PM Qu Wenruo wrote: finit_module() and init_module() system calls have clear specification to only accept valid ELF image. Although we try finit_module() on compressed modules to let the kernel determine if it's an ELF image, but it's not ideal, especially when newer kernel will complain when some invalid files/memory is passed in. Treat the kernel better by just doing a very basic ELF header check before calling finit_module(). Signed-off-by: Qu Wenruo What is the reason for not letting the kernel do all the ELF sanity checks? Performance? Security? For now this looks like extra code to busybox without obvious benefits. To avoid possible "Invalid ELF header" error message from kernel. Since those system calls are only to accept ELF header, kernel has its right to info the caller that it got some invalid ELF header (even if it's just compressed file). Or did you mean, busybox pursues size so much that it doesn't matter to not follow system call spec? Thanks, Qu ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2 1/2] modutils: check ELF header before calling finit_module()
On Sun, Jan 3, 2021 at 12:11 PM Qu Wenruo wrote: > > finit_module() and init_module() system calls have clear specification > to only accept valid ELF image. > > Although we try finit_module() on compressed modules to let the kernel > determine if it's an ELF image, but it's not ideal, especially when > newer kernel will complain when some invalid files/memory is passed in. > > Treat the kernel better by just doing a very basic ELF header check > before calling finit_module(). > > Signed-off-by: Qu Wenruo What is the reason for not letting the kernel do all the ELF sanity checks? Performance? Security? For now this looks like extra code to busybox without obvious benefits. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
