| Message ID | 20190806043729.5562-1-yamada.masahiro@socionext.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] kbuild: re-implement detection of CONFIG options leaked to user-space | expand |
On Tue, Aug 6, 2019 at 6:38 AM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > I was playing with sed yesterday, but the resulted code might be unreadable. > > Sed scripts tend to be somewhat unreadable. > I just wondered which language is appropriate for this? > Maybe perl, or what else? I am not good at perl, though. I like the sed version, in particular as it seems to do the job and I'm not volunteering to write it in anything else. > Maybe, it will be better to fix existing warnings > before enabling this check. Yes, absolutely. > If somebody takes a closer look at them, that would be great. Let's see: > warning: include/uapi/linux/elfcore.h: leaks CONFIG_BINFMT_ELF_FDPIC to user-space This one is nontrivial, since it defines two incompatible layouts for this structure, and the fdpic version is currently not usable at all from user space. Also, the definition breaks configurations that have both CONFIG_BINFMT_ELF and CONFIG_BINFMT_ELF_FDPIC enabled, which has become possible with commit 382e67aec6a7 ("ARM: enable elf_fdpic on systems with an MMU"). The best way forward I see is to duplicate the structure definition, adding a new 'struct elf_fdpic_prstatus', and using that in fs/binfmt_elf_fdpic.c. The same change is required in include/linux/elfcore-compat.h. > warning: include/uapi/linux/atmdev.h: leaks CONFIG_COMPAT to user-space The "#define COMPAT_ATM_ADDPARTY" can be moved to include/linux/atmdev.h, it's not needed in the uapi header > warning: include/uapi/linux/raw.h: leaks CONFIG_MAX_RAW_DEVS to user-space This has never been usable, I'd just remove MAX_RAW_MINORS and change drivers/char/raw.c to use CONFIG_MAX_RAW_DEVS > warning: include/uapi/linux/pktcdvd.h: leaks CONFIG_CDROM_PKTCDVD_WCACHE to user-space USE_WCACHING can be moved to drivers/block/pktcdvd.c > warning: include/uapi/linux/eventpoll.h: leaks CONFIG_PM_SLEEP to user-space ep_take_care_of_epollwakeup() should not be in the header at all I think. Commit 95f19f658ce1 ("epoll: drop EPOLLWAKEUP if PM_SLEEP is disabled") was wrong to move it out of fs/eventpoll.c, and I'd just move it back as an inline function. (Added Amit to Cc for clarification). > warning: include/uapi/linux/hw_breakpoint.h: leaks CONFIG_HAVE_MIXED_BREAKPOINTS_REGS to user-space enum bp_type_idx started out in kernel/events/hw_breakpoint.c and was later moved to a header which then became public. I don't think it was ever meant to be public though. We either want to move it back, or change the CONFIG_HAVE_MIXED_BREAKPOINTS_REGS macro to an __ARCH_HAVE_MIXED_BREAKPOINTS_REGS that is explicitly set in a header file by x86 and superh. > warning: include/uapi/asm-generic/fcntl.h: leaks CONFIG_64BIT to user-space The #ifdef could just be changed to #if __BITS_PER_LONG == 32 We could also do this differently, given that most 64-bit architectures define the same macros in their arch/*/include/asm/compat.h files (parisc and mips use different values). > warning: arch/x86/include/uapi/asm/mman.h: leaks CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS to user-space I think arch_vm_get_page_prot should not be in the uapi header, other architectures have it in arch/powerpc/include/asm/mman.h > warning: arch/x86/include/uapi/asm/auxvec.h: leaks CONFIG_IA32_EMULATION to user-space > warning: arch/x86/include/uapi/asm/auxvec.h: leaks CONFIG_X86_64 to user-space It looks like this definition has always been wrong, across several changes that made it wrong in different ways. AT_VECTOR_SIZE_ARCH is supposed to define the size of the extra aux vectors, which is meant to be '2' for i386 tasks, and '1' for x86_64 tasks, regardless of how the kernel is configured. I looked at this for a bit but it's hard to tell how to fix this without introducing possible regressions. Note how 'mm->saved_auxv' uses this size and gets copied between kernel and user space. Arnd
Hi Arnd, On Tue, Aug 6, 2019 at 6:00 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Tue, Aug 6, 2019 at 6:38 AM Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: > > > > I was playing with sed yesterday, but the resulted code might be unreadable. > > > > Sed scripts tend to be somewhat unreadable. > > I just wondered which language is appropriate for this? > > Maybe perl, or what else? I am not good at perl, though. > > I like the sed version, in particular as it seems to do the job and > I'm not volunteering to write it in anything else. > > > Maybe, it will be better to fix existing warnings > > before enabling this check. > > Yes, absolutely. > > > If somebody takes a closer look at them, that would be great. > > Let's see: Thanks for looking into these! If possible, could you please send patches to fix them? We can start with low-hanging fruits to reduce the number of warnings. Thank you.
On Tue, Aug 06, 2019 at 11:00:19AM +0200, Arnd Bergmann wrote: > > I was playing with sed yesterday, but the resulted code might be unreadable. > > > > Sed scripts tend to be somewhat unreadable. > > I just wondered which language is appropriate for this? > > Maybe perl, or what else? I am not good at perl, though. > > I like the sed version, in particular as it seems to do the job and > I'm not volunteering to write it in anything else. Did anyone not like sed? I have to say I do like scripts using sed and awk because they are fairly readable and avoid dependencies on "big" scripting language and their optional modules that sooner or later get pulled in. > This one is nontrivial, since it defines two incompatible layouts for > this structure, > and the fdpic version is currently not usable at all from user space. Also, > the definition breaks configurations that have both CONFIG_BINFMT_ELF > and CONFIG_BINFMT_ELF_FDPIC enabled, which has become possible > with commit 382e67aec6a7 ("ARM: enable elf_fdpic on systems with an MMU"). > > The best way forward I see is to duplicate the structure definition, adding > a new 'struct elf_fdpic_prstatus', and using that in fs/binfmt_elf_fdpic.c. > The same change is required in include/linux/elfcore-compat.h. Yeah, this is a mess. David Howells suggested something similar when I brought the issue to his attention last time.
diff --git a/scripts/headers_install.sh b/scripts/headers_install.sh index bbaf29386995..73d95e457090 100755 --- a/scripts/headers_install.sh +++ b/scripts/headers_install.sh @@ -41,5 +41,34 @@ sed -E -e ' scripts/unifdef -U__KERNEL__ -D__EXPORTED_HEADERS__ $TMPFILE > $OUTFILE [ $? -gt 1 ] && exit 1 +# Remove /* ... */ style comments, and find CONFIG_ references in code +configs=$(sed -e ' +:comment + s:/\*[^*][^*]*:/*: + s:/\*\*\**\([^/]\):/*\1: + t comment + s:/\*\*/: : + t comment + /\/\*/! b check + N + b comment +:print + P + D +:check + s:^[^[:alnum:]_][^[:alnum:]_]*:: + t check + s:^\(CONFIG_[[:alnum:]_]*\):\1\n: + t print + s:^[[:alnum:]_][[:alnum:]_]*:: + t check + d +' $OUTFILE) + +for c in $configs +do + echo "warning: $INFILE: leaks $c to user-space" >&2 +done + rm -f $TMPFILE trap - EXIT
scripts/headers_check.pl can detect references to CONFIG options in exported headers, but it has been disabled for more than a decade. Reverting commit 7e3fa5614117 ("kbuild: drop check for CONFIG_ in headers_check") would emit the following warnings for headers_check on x86: usr/include/mtd/ubi-user.h:283: leaks CONFIG_MTD_UBI_BEB_LIMIT to userspace where it is not valid usr/include/linux/elfcore.h:62: leaks CONFIG_BINFMT_ELF_FDPIC to userspace where it is not valid usr/include/linux/atmdev.h:104: leaks CONFIG_COMPAT to userspace where it is not valid usr/include/linux/raw.h:17: leaks CONFIG_MAX_RAW_DEVS to userspace where it is not valid usr/include/linux/pktcdvd.h:37: leaks CONFIG_CDROM_PKTCDVD_WCACHE to userspace where it is not valid usr/include/linux/videodev2.h:2465: leaks CONFIG_VIDEO_ADV_DEBUG to userspace where it is not valid usr/include/linux/bpf.h:249: leaks CONFIG_EFFICIENT_UNALIGNED_ACCESS to userspace where it is not valid usr/include/linux/bpf.h:819: leaks CONFIG_CGROUP_NET_CLASSID to userspace where it is not valid usr/include/linux/bpf.h:1011: leaks CONFIG_IP_ROUTE_CLASSID to userspace where it is not valid usr/include/linux/bpf.h:1742: leaks CONFIG_BPF_KPROBE_OVERRIDE to userspace where it is not valid usr/include/linux/bpf.h:1747: leaks CONFIG_FUNCTION_ERROR_INJECTION to userspace where it is not valid usr/include/linux/bpf.h:1936: leaks CONFIG_XFRM to userspace where it is not valid usr/include/linux/bpf.h:2184: leaks CONFIG_BPF_LIRC_MODE2 to userspace where it is not valid usr/include/linux/bpf.h:2210: leaks CONFIG_BPF_LIRC_MODE2 to userspace where it is not valid usr/include/linux/bpf.h:2227: leaks CONFIG_SOCK_CGROUP_DATA to userspace where it is not valid usr/include/linux/bpf.h:2311: leaks CONFIG_NET to userspace where it is not valid usr/include/linux/bpf.h:2348: leaks CONFIG_NET to userspace where it is not valid usr/include/linux/bpf.h:2422: leaks CONFIG_BPF_LIRC_MODE2 to userspace where it is not valid usr/include/linux/bpf.h:2528: leaks CONFIG_NET to userspace where it is not valid usr/include/linux/eventpoll.h:82: leaks CONFIG_PM_SLEEP to userspace where it is not valid usr/include/linux/hw_breakpoint.h:27: leaks CONFIG_HAVE_MIXED_BREAKPOINTS_REGS to userspace where it is not valid usr/include/linux/cm4000_cs.h:26: leaks CONFIG_COMPAT to userspace where it is not valid usr/include/linux/pkt_cls.h:301: leaks CONFIG_NET_CLS_ACT to userspace where it is not valid usr/include/asm-generic/unistd.h:651: leaks CONFIG_MMU to userspace where it is not valid usr/include/asm-generic/fcntl.h:119: leaks CONFIG_64BIT to userspace where it is not valid usr/include/asm-generic/bitsperlong.h:9: leaks CONFIG_64BIT to userspace where it is not valid usr/include/asm/e820.h:14: leaks CONFIG_NODES_SHIFT to userspace where it is not valid usr/include/asm/e820.h:39: leaks CONFIG_X86_PMEM_LEGACY to userspace where it is not valid usr/include/asm/e820.h:49: leaks CONFIG_INTEL_TXT to userspace where it is not valid usr/include/asm/mman.h:7: leaks CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS to userspace where it is not valid usr/include/asm/auxvec.h:14: leaks CONFIG_IA32_EMULATION to userspace where it is not valid Most of these are false positives because scripts/headers_check.pl parses comment lines. It is also false negative. arch/x86/include/uapi/asm/auxvec.h contains CONFIG_IA32_EMULATION and CONFIG_X86_64, but the only former is reported. It would be possible to fix scripts/headers_check.pl, of course. However, we already have some duplicated checks between headers_check and CONFIG_UAPI_HEADER_TEST. At this moment of time, there are still dozens of headers excluded from the header test (usr/include/Makefile), but we might be able to remove headers_check when the time comes. I re-implemented it in scripts/headers_install.sh by using sed because the most of code in scripts/headers_install.sh is written is sed. This patch works like this: [1] Run scripts/unifdef first because we need to drop the code surrounded by #ifdef __KERNEL__ ... #endif [2] Remove all C style comments. The sed code is somewhat complicated since we need to deal with both single and multi line comments. Precisely speaking, a comment block is replaced with a space just in case. CONFIG_FOO/* this is a comment */CONFIG_BAR should be converted into: CONFIG_FOO CONFIG_BAR instead of: CONFIG_FOOCONFIG_BAR [3] Match CONFIG_... pattern. It correctly matches to all CONFIG options that appear in a single line. After this commit, you will see the following warnings, all of which are real ones. warning: include/uapi/linux/elfcore.h: leaks CONFIG_BINFMT_ELF_FDPIC to user-space warning: include/uapi/linux/atmdev.h: leaks CONFIG_COMPAT to user-space warning: include/uapi/linux/raw.h: leaks CONFIG_MAX_RAW_DEVS to user-space warning: include/uapi/linux/pktcdvd.h: leaks CONFIG_CDROM_PKTCDVD_WCACHE to user-space warning: include/uapi/linux/eventpoll.h: leaks CONFIG_PM_SLEEP to user-space warning: include/uapi/linux/hw_breakpoint.h: leaks CONFIG_HAVE_MIXED_BREAKPOINTS_REGS to user-space warning: include/uapi/asm-generic/fcntl.h: leaks CONFIG_64BIT to user-space warning: arch/x86/include/uapi/asm/mman.h: leaks CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS to user-space warning: arch/x86/include/uapi/asm/auxvec.h: leaks CONFIG_IA32_EMULATION to user-space warning: arch/x86/include/uapi/asm/auxvec.h: leaks CONFIG_X86_64 to user-space Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- I was playing with sed yesterday, but the resulted code might be unreadable. Sed scripts tend to be somewhat unreadable. I just wondered which language is appropriate for this? Maybe perl, or what else? I am not good at perl, though. Maybe, it will be better to fix existing warnings before enabling this check. If somebody takes a closer look at them, that would be great. scripts/headers_install.sh | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)