| Message ID | 20240809122007.1220219-2-masahiroy@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: Do not include <linux/*.h> from host programs (+ extra clean-up) | expand |
On Aug 9, 2024 Masahiro Yamada <masahiroy@kernel.org> wrote: > > Commit bfc5e3a6af39 ("selinux: use the kernel headers when building > scripts/selinux") is not the right thing to do. > > It is clear from the warning in include/uapi/linux/types.h: > > #ifndef __EXPORTED_HEADERS__ > #warning "Attempt to use kernel headers from user space, see https://kernelnewbies.org/KernelHeaders" > #endif /* __EXPORTED_HEADERS__ */ > > If you are inclined to define __EXPORTED_HEADERS__, you are likely doing > wrong. > > Adding the comment: > > /* NOTE: we really do want to use the kernel headers here */ > > does not justify the hack in any way. > > Currently, <linux/*.h> headers are included for the following purposes: > > - <linux/capability.h> is included to check CAP_LAST_CAP > - <linux/socket.h> in included to check PF_MAX > > We can skip these checks when building host programs, as they will > be eventually tested when building the kernel space. > > I got rid of <linux/stddef.h> from initial_sid_to_string.h because > it is likely that NULL is already defined. If you insist on making > it self-contained, you can add the following: > > #ifdef __KERNEL__ > #include <linux/stddef.h> > #else > #include <stddef.h> > #endif > > scripts/selinux/mdp/mdp.c still includes <linux/kconfig.h>, which is > also discouraged and should be fixed by a follow-up refactoring. > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > --- > scripts/selinux/genheaders/Makefile | 4 +--- > scripts/selinux/genheaders/genheaders.c | 3 --- > scripts/selinux/mdp/Makefile | 2 +- > scripts/selinux/mdp/mdp.c | 4 ---- > security/selinux/include/classmap.h | 19 ++++++++++++------- > .../selinux/include/initial_sid_to_string.h | 2 -- > 6 files changed, 14 insertions(+), 20 deletions(-) I'm not going to argue with the general idea behind this commit, but I am going to complain about the writing style in that commit description, it's not something I'm going to pull via the SELinux tree and I'm going to object to it going in via any other tree. Please rewrite the commit description to simply state the problem (e.g. "we do not need to include the kernel headers when building the userspace SELinux scripts") and the fix (e.g "we resolve this by making the kernel specific portions protected by a __KERNEL__ macro check"). If you want to preserve the snarky commentary you can do so in the cover letter. > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h > index 7229c9bf6c27..518209e1beb0 100644 > --- a/security/selinux/include/classmap.h > +++ b/security/selinux/include/classmap.h > @@ -1,8 +1,5 @@ > /* SPDX-License-Identifier: GPL-2.0 */ > > -#include <linux/capability.h> > -#include <linux/socket.h> > - > #define COMMON_FILE_SOCK_PERMS \ > "ioctl", "read", "write", "create", "getattr", "setattr", "lock", \ > "relabelfrom", "relabelto", "append", "map" > @@ -36,10 +33,6 @@ > "mac_override", "mac_admin", "syslog", "wake_alarm", "block_suspend", \ > "audit_read", "perfmon", "bpf", "checkpoint_restore" > > -#if CAP_LAST_CAP > CAP_CHECKPOINT_RESTORE > -#error New capability defined, please update COMMON_CAP2_PERMS. > -#endif > - > /* > * Note: The name for any socket class should be suffixed by "socket", > * and doesn't contain more than one substr of "socket". > @@ -181,6 +174,18 @@ const struct security_class_mapping secclass_map[] = { > { NULL } > }; > > +#ifdef __KERNEL__ /* avoid this check when building host programs */ > + > +#include <linux/capability.h> > + > +#if CAP_LAST_CAP > CAP_CHECKPOINT_RESTORE > +#error New capability defined, please update COMMON_CAP2_PERMS. > +#endif > + > +#include <linux/socket.h> > + > #if PF_MAX > 46 > #error New address family defined, please update secclass_map. > #endif > + > +#endif /* __KERNEL__ */ Please keep both the CAP_LAST_CAP and PF_MAX checks where they are now, closer to the affected code. > diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h > index 99b353b2abb4..f683a78b21fd 100644 > --- a/security/selinux/include/initial_sid_to_string.h > +++ b/security/selinux/include/initial_sid_to_string.h > @@ -1,7 +1,5 @@ > /* SPDX-License-Identifier: GPL-2.0 */ > > -#include <linux/stddef.h> Yes, we don't need the kernel's stddef.h here, but I would like us to have some type of stdddef.h included here so we have NULL defined. > static const char *const initial_sid_to_string[] = { > NULL, /* zero placeholder, not used */ > "kernel", /* kernel / SECINITSID_KERNEL */ > -- > 2.43.0 -- paul-moore.com
diff --git a/scripts/selinux/genheaders/Makefile b/scripts/selinux/genheaders/Makefile index 1faf7f07e8db..866f60e78882 100644 --- a/scripts/selinux/genheaders/Makefile +++ b/scripts/selinux/genheaders/Makefile @@ -1,5 +1,3 @@ # SPDX-License-Identifier: GPL-2.0 hostprogs-always-y += genheaders -HOST_EXTRACFLAGS += \ - -I$(srctree)/include/uapi -I$(srctree)/include \ - -I$(srctree)/security/selinux/include +HOST_EXTRACFLAGS += -I$(srctree)/security/selinux/include diff --git a/scripts/selinux/genheaders/genheaders.c b/scripts/selinux/genheaders/genheaders.c index 15520806889e..3834d7eb0af6 100644 --- a/scripts/selinux/genheaders/genheaders.c +++ b/scripts/selinux/genheaders/genheaders.c @@ -1,8 +1,5 @@ // SPDX-License-Identifier: GPL-2.0 -/* NOTE: we really do want to use the kernel headers here */ -#define __EXPORTED_HEADERS__ - #include <stdio.h> #include <stdlib.h> #include <unistd.h> diff --git a/scripts/selinux/mdp/Makefile b/scripts/selinux/mdp/Makefile index d61058ddd15c..673782e3212f 100644 --- a/scripts/selinux/mdp/Makefile +++ b/scripts/selinux/mdp/Makefile @@ -1,7 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 hostprogs-always-y += mdp HOST_EXTRACFLAGS += \ - -I$(srctree)/include/uapi -I$(srctree)/include \ + -I$(srctree)/include \ -I$(srctree)/security/selinux/include -I$(objtree)/include clean-files := policy.* file_contexts diff --git a/scripts/selinux/mdp/mdp.c b/scripts/selinux/mdp/mdp.c index 1415604c3d24..52365921c043 100644 --- a/scripts/selinux/mdp/mdp.c +++ b/scripts/selinux/mdp/mdp.c @@ -11,10 +11,6 @@ * Authors: Serge E. Hallyn <serue@us.ibm.com> */ - -/* NOTE: we really do want to use the kernel headers here */ -#define __EXPORTED_HEADERS__ - #include <stdio.h> #include <stdlib.h> #include <unistd.h> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h index 7229c9bf6c27..518209e1beb0 100644 --- a/security/selinux/include/classmap.h +++ b/security/selinux/include/classmap.h @@ -1,8 +1,5 @@ /* SPDX-License-Identifier: GPL-2.0 */ -#include <linux/capability.h> -#include <linux/socket.h> - #define COMMON_FILE_SOCK_PERMS \ "ioctl", "read", "write", "create", "getattr", "setattr", "lock", \ "relabelfrom", "relabelto", "append", "map" @@ -36,10 +33,6 @@ "mac_override", "mac_admin", "syslog", "wake_alarm", "block_suspend", \ "audit_read", "perfmon", "bpf", "checkpoint_restore" -#if CAP_LAST_CAP > CAP_CHECKPOINT_RESTORE -#error New capability defined, please update COMMON_CAP2_PERMS. -#endif - /* * Note: The name for any socket class should be suffixed by "socket", * and doesn't contain more than one substr of "socket". @@ -181,6 +174,18 @@ const struct security_class_mapping secclass_map[] = { { NULL } }; +#ifdef __KERNEL__ /* avoid this check when building host programs */ + +#include <linux/capability.h> + +#if CAP_LAST_CAP > CAP_CHECKPOINT_RESTORE +#error New capability defined, please update COMMON_CAP2_PERMS. +#endif + +#include <linux/socket.h> + #if PF_MAX > 46 #error New address family defined, please update secclass_map. #endif + +#endif /* __KERNEL__ */ diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h index 99b353b2abb4..f683a78b21fd 100644 --- a/security/selinux/include/initial_sid_to_string.h +++ b/security/selinux/include/initial_sid_to_string.h @@ -1,7 +1,5 @@ /* SPDX-License-Identifier: GPL-2.0 */ -#include <linux/stddef.h> - static const char *const initial_sid_to_string[] = { NULL, /* zero placeholder, not used */ "kernel", /* kernel / SECINITSID_KERNEL */
Commit bfc5e3a6af39 ("selinux: use the kernel headers when building scripts/selinux") is not the right thing to do. It is clear from the warning in include/uapi/linux/types.h: #ifndef __EXPORTED_HEADERS__ #warning "Attempt to use kernel headers from user space, see https://kernelnewbies.org/KernelHeaders" #endif /* __EXPORTED_HEADERS__ */ If you are inclined to define __EXPORTED_HEADERS__, you are likely doing wrong. Adding the comment: /* NOTE: we really do want to use the kernel headers here */ does not justify the hack in any way. Currently, <linux/*.h> headers are included for the following purposes: - <linux/capability.h> is included to check CAP_LAST_CAP - <linux/socket.h> in included to check PF_MAX We can skip these checks when building host programs, as they will be eventually tested when building the kernel space. I got rid of <linux/stddef.h> from initial_sid_to_string.h because it is likely that NULL is already defined. If you insist on making it self-contained, you can add the following: #ifdef __KERNEL__ #include <linux/stddef.h> #else #include <stddef.h> #endif scripts/selinux/mdp/mdp.c still includes <linux/kconfig.h>, which is also discouraged and should be fixed by a follow-up refactoring. Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> --- scripts/selinux/genheaders/Makefile | 4 +--- scripts/selinux/genheaders/genheaders.c | 3 --- scripts/selinux/mdp/Makefile | 2 +- scripts/selinux/mdp/mdp.c | 4 ---- security/selinux/include/classmap.h | 19 ++++++++++++------- .../selinux/include/initial_sid_to_string.h | 2 -- 6 files changed, 14 insertions(+), 20 deletions(-)