| Message ID | 1479482589-3889-1-git-send-email-sds@tycho.nsa.gov (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, Nov 18, 2016 at 10:23 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > When a new capability is defined, SELinux needs to be updated. > Trigger a build error if a new capability is defined without > corresponding update to security/selinux/include/classmap.h's > COMMON_CAP2_PERMS. This is similar to BUILD_BUG_ON() guards > in the SELinux nlmsgtab code to ensure that SELinux tracks > new netlink message types as needed. > > Note that there is already a similar build guard in > security/selinux/hooks.c to detect when more than 64 > capabilities are defined, since that will require adding > a third capability class to SELinux. > > A nicer way to do this would be to extend scripts/selinux/genheaders > or a similar tool to auto-generate the necessary definitions and code > for SELinux capability checking from include/uapi/linux/capability.h. > AppArmor does something similar in its Makefile, although it only > needs to generate a single table of names. That is left as future work. > > Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> > --- > security/selinux/include/classmap.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h > index 1f1f4b2..e2d4ad3a 100644 > --- a/security/selinux/include/classmap.h > +++ b/security/selinux/include/classmap.h > @@ -24,6 +24,10 @@ > #define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \ > "wake_alarm", "block_suspend", "audit_read" > > +#if CAP_LAST_CAP > CAP_AUDIT_READ > +#error New capability defined, please update COMMON_CAP2_PERMS. > +#endif I think the obvious question here is why not use BUILD_BUG_ON() here? I understand that it can be disabled, but it seems like the "good neighbor" option compared to the #error pragma.
On 11/20/2016 05:29 PM, Paul Moore wrote: > On Fri, Nov 18, 2016 at 10:23 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >> When a new capability is defined, SELinux needs to be updated. >> Trigger a build error if a new capability is defined without >> corresponding update to security/selinux/include/classmap.h's >> COMMON_CAP2_PERMS. This is similar to BUILD_BUG_ON() guards >> in the SELinux nlmsgtab code to ensure that SELinux tracks >> new netlink message types as needed. >> >> Note that there is already a similar build guard in >> security/selinux/hooks.c to detect when more than 64 >> capabilities are defined, since that will require adding >> a third capability class to SELinux. >> >> A nicer way to do this would be to extend scripts/selinux/genheaders >> or a similar tool to auto-generate the necessary definitions and code >> for SELinux capability checking from include/uapi/linux/capability.h. >> AppArmor does something similar in its Makefile, although it only >> needs to generate a single table of names. That is left as future work. >> >> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> >> --- >> security/selinux/include/classmap.h | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h >> index 1f1f4b2..e2d4ad3a 100644 >> --- a/security/selinux/include/classmap.h >> +++ b/security/selinux/include/classmap.h >> @@ -24,6 +24,10 @@ >> #define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \ >> "wake_alarm", "block_suspend", "audit_read" >> >> +#if CAP_LAST_CAP > CAP_AUDIT_READ >> +#error New capability defined, please update COMMON_CAP2_PERMS. >> +#endif > > I think the obvious question here is why not use BUILD_BUG_ON() here? > I understand that it can be disabled, but it seems like the "good > neighbor" option compared to the #error pragma. I wanted the error to be triggered in the file that needs to be updated, and preferably close to the line that needs to be updated. BUILD_BUG_ON() and friends can only be used within a function, and there is no function in view in classmap.h. We could put a BUILD_BUG_ON() in one of the functions that use secclass_map[], e.g. avc_dump_av(), but it will be less directly correlated to what needs to be updated - it won't be the correct file.
On Mon, Nov 21, 2016 at 9:49 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 11/20/2016 05:29 PM, Paul Moore wrote: >> On Fri, Nov 18, 2016 at 10:23 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >>> When a new capability is defined, SELinux needs to be updated. >>> Trigger a build error if a new capability is defined without >>> corresponding update to security/selinux/include/classmap.h's >>> COMMON_CAP2_PERMS. This is similar to BUILD_BUG_ON() guards >>> in the SELinux nlmsgtab code to ensure that SELinux tracks >>> new netlink message types as needed. >>> >>> Note that there is already a similar build guard in >>> security/selinux/hooks.c to detect when more than 64 >>> capabilities are defined, since that will require adding >>> a third capability class to SELinux. >>> >>> A nicer way to do this would be to extend scripts/selinux/genheaders >>> or a similar tool to auto-generate the necessary definitions and code >>> for SELinux capability checking from include/uapi/linux/capability.h. >>> AppArmor does something similar in its Makefile, although it only >>> needs to generate a single table of names. That is left as future work. >>> >>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> >>> --- >>> security/selinux/include/classmap.h | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h >>> index 1f1f4b2..e2d4ad3a 100644 >>> --- a/security/selinux/include/classmap.h >>> +++ b/security/selinux/include/classmap.h >>> @@ -24,6 +24,10 @@ >>> #define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \ >>> "wake_alarm", "block_suspend", "audit_read" >>> >>> +#if CAP_LAST_CAP > CAP_AUDIT_READ >>> +#error New capability defined, please update COMMON_CAP2_PERMS. >>> +#endif >> >> I think the obvious question here is why not use BUILD_BUG_ON() here? >> I understand that it can be disabled, but it seems like the "good >> neighbor" option compared to the #error pragma. > > I wanted the error to be triggered in the file that needs to be updated, > and preferably close to the line that needs to be updated. > BUILD_BUG_ON() and friends can only be used within a function, and there > is no function in view in classmap.h. We could put a BUILD_BUG_ON() in > one of the functions that use secclass_map[], e.g. avc_dump_av(), but it > will be less directly correlated to what needs to be updated - it won't > be the correct file. Okay, I see the value in keeping the error close to the root cause. Merged.
On Mon, Dec 19, 2016 at 8:35 PM, Paul Moore <paul@paul-moore.com> wrote: > On Mon, Dec 19, 2016 at 9:24 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >> On Sun, 2016-12-18 at 21:06 +0100, Nicolas Iooss wrote: >>> Hello, >>> This patch made the compiler I am using to build the kernel (clang) >>> report two new warnings when building >>> scripts/selinux/genheaders/genheaders.c and >>> scripts/selinux/mdp/mdp.c: >>> >>> 'CAP_LAST_CAP' is not defined, evaluates to 0 [-Wundef] >>> 'CAP_AUDIT_READ' is not defined, evaluates to 0 [-Wundef] >>> >>> Even though this is not detected by gcc, it seems like a bug to >>> compare >>> undefined values. There is no issue where classmap.h is included from >>> security/selinux/avc.c because include/uapi/linux/capability.h got >>> included too. >>> >>> I see two ways of fixing these warnings: either by defining the >>> capability values in genheaders and mdp by adding #include >>> <linux/capability.h>, or by adding "defined(__KERNEL__) &&" before >>> the >>> test so that it is only processed from kernel code (avc.c). How would >>> you like this to be fixed? >> >> I suppose we ought to #include <uapi/linux/capability.h> in classmap.h. > > Yep. Unless one of you wants to beat me to it, I'll put a quick patch > together tomorrow. See the patch I just posted to the list. It turns out it wasn't quite that easy due to conflicts between the kernel and system among the various nested includes, but I think the posted patch should solve everything, if not please let me know. If I don't hear anything, I'll push this up to James later this week (tomorrow?) for inclusion into v4.10.
On 20/12/16 18:49, Paul Moore wrote: > On Mon, Dec 19, 2016 at 8:35 PM, Paul Moore <paul@paul-moore.com> wrote: >> On Mon, Dec 19, 2016 at 9:24 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >>> On Sun, 2016-12-18 at 21:06 +0100, Nicolas Iooss wrote: >>>> Hello, >>>> This patch made the compiler I am using to build the kernel (clang) >>>> report two new warnings when building >>>> scripts/selinux/genheaders/genheaders.c and >>>> scripts/selinux/mdp/mdp.c: >>>> >>>> 'CAP_LAST_CAP' is not defined, evaluates to 0 [-Wundef] >>>> 'CAP_AUDIT_READ' is not defined, evaluates to 0 [-Wundef] >>>> >>>> Even though this is not detected by gcc, it seems like a bug to >>>> compare >>>> undefined values. There is no issue where classmap.h is included from >>>> security/selinux/avc.c because include/uapi/linux/capability.h got >>>> included too. >>>> >>>> I see two ways of fixing these warnings: either by defining the >>>> capability values in genheaders and mdp by adding #include >>>> <linux/capability.h>, or by adding "defined(__KERNEL__) &&" before >>>> the >>>> test so that it is only processed from kernel code (avc.c). How would >>>> you like this to be fixed? >>> >>> I suppose we ought to #include <uapi/linux/capability.h> in classmap.h. >> >> Yep. Unless one of you wants to beat me to it, I'll put a quick patch >> together tomorrow. > > See the patch I just posted to the list. It turns out it wasn't quite > that easy due to conflicts between the kernel and system among the > various nested includes, but I think the posted patch should solve > everything, if not please let me know. If I don't hear anything, I'll > push this up to James later this week (tomorrow?) for inclusion into > v4.10. Hello, I confirm the patch you posted fixed the warnings I had. Nevertheless when I take a look at which file got included by scripts/selinux/mdp/mdp.c, it appears that classmap.h includes the system header /usr/include/linux/capability.h instead of include/uapi/linux/capability.h (unlike genheaders, which included the last file). Is this something you wanted? Thanks! Nicolas
On Wed, Dec 21, 2016 at 10:07 AM, Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > On 20/12/16 18:49, Paul Moore wrote: >> See the patch I just posted to the list. It turns out it wasn't quite >> that easy due to conflicts between the kernel and system among the >> various nested includes, but I think the posted patch should solve >> everything, if not please let me know. If I don't hear anything, I'll >> push this up to James later this week (tomorrow?) for inclusion into >> v4.10. > > Hello, > I confirm the patch you posted fixed the warnings I had. Nevertheless > when I take a look at which file got included by > scripts/selinux/mdp/mdp.c, it appears that classmap.h includes the > system header /usr/include/linux/capability.h instead of > include/uapi/linux/capability.h (unlike genheaders, which included the > last file). Is this something you wanted? Probably not, thanks for pointing this out; give me a few minutes and I'll post a v2 patch.
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h index 1f1f4b2..e2d4ad3a 100644 --- a/security/selinux/include/classmap.h +++ b/security/selinux/include/classmap.h @@ -24,6 +24,10 @@ #define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \ "wake_alarm", "block_suspend", "audit_read" +#if CAP_LAST_CAP > CAP_AUDIT_READ +#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".
When a new capability is defined, SELinux needs to be updated. Trigger a build error if a new capability is defined without corresponding update to security/selinux/include/classmap.h's COMMON_CAP2_PERMS. This is similar to BUILD_BUG_ON() guards in the SELinux nlmsgtab code to ensure that SELinux tracks new netlink message types as needed. Note that there is already a similar build guard in security/selinux/hooks.c to detect when more than 64 capabilities are defined, since that will require adding a third capability class to SELinux. A nicer way to do this would be to extend scripts/selinux/genheaders or a similar tool to auto-generate the necessary definitions and code for SELinux capability checking from include/uapi/linux/capability.h. AppArmor does something similar in its Makefile, although it only needs to generate a single table of names. That is left as future work. Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> --- security/selinux/include/classmap.h | 4 ++++ 1 file changed, 4 insertions(+)