mm.h, security.h, key.h and preventing namespace poisoning
Subsystems (4)
RFC PATCH 1/1 mm.h, security.h, key.h and preventing namespace poisoning
Folks,
I just updated a git tree and started getting errors on a "copy_keys" macro warning.
The code I've been working on uses a ->copy_keys() method for copying the keys in a btree block from one place to another. I've been working on this code for a while
(http://oss.sgi.com/archives/xfs/2007-11/msg00046.html) and keep the tree I'm working in reletively up to date (lags linus by a couple of weeks at most). The update I did this afternoon gave a conflict warning with the macro in include/linux/key.h.
Given that I'm not directly including key.h anywhere in the XFS code, I'm getting the namespace polluted indirectly from some other include that is necessary.
As it turns out, this commit from 13 days ago:
included security.h in mm.h and that is how I'm seeing the namespace poisoning coming from key.h when !CONFIG_KEY.
Including security.h in mm.h means much wider includes for pretty much the entire kernel, and it opens up namespace issues like this that never previously existed.
The patch below (only tested for !CONFIG_KEYS && !CONFIG_SECURITY) moves security.h into the mmap.c and nommu.c files that need it so it doesn't end up with kernel wide scope.
Comments?
--- include/linux/mm.h | 16 ---------------- include/linux/security.h | 26 +++++++++++++++++++++++++- mm/mmap.c | 1 + mm/nommu.c | 1 + 4 files changed, 27 insertions(+), 17 deletions(-)
include/linux/mm.h +0 -16
@@ -12,7 +12,6 @@#include <linux/prio_tree.h> #include <linux/debug_locks.h> #include <linux/mm_types.h> -#include <linux/security.h> struct mempolicy ;struct anon_vma ;
@@ -514,21 +513,6 @@ static inline void set_page_links(struct page *page, enum zone_type zone, }/* -* If a hint addr is less than mmap_min_addr change hint to be as -* low as possible but still greater than mmap_min_addr -*/ -static inline unsigned long round_hint_to_min (unsigned long hint) -{ -#ifdef CONFIG_SECURITY - hint &=PAGE_MASK ; -if (((void *)hint !=NULL ) && - (hint < mmap_min_addr)) -return PAGE_ALIGN (mmap_min_addr); -#endif -return hint; -} - -/* * Some inline functions in vmstat.h depend on page_zone() */ #include <linux/vmstat.h>
include/linux/security.h +25 -1
@@ -2568,6 +2568,19 @@ void security_key_free(struct key *key);int security_key_permission (key_ref_t key_ref,struct task_struct *context,key_perm_t perm); +/* +* If a hint addr is less than mmap_min_addr change hint to be as +* low as possible but still greater than mmap_min_addr +*/ +static inline unsigned long round_hint_to_min (unsigned long hint) +{ + hint &=PAGE_MASK ; +if (((void *)hint !=NULL ) && + (hint < mmap_min_addr)) +return PAGE_ALIGN (mmap_min_addr); +return hint; +} +#else static inline int security_key_alloc(struct key *key,
@@ -2588,7 +2601,18 @@ static inline int security_key_permission(key_ref_t key_ref,return 0 ; } -#endif +static inline unsigned long round_hint_to_min (unsigned long hint) +{ +return hint; +} + +#endif /* CONFIG_SECURITY */ + +#else /* !CONFIG_KEYS */ +static inline unsigned long round_hint_to_min (unsigned long hint) +{ +return hint; +}#endif /* CONFIG_KEYS */ #endif /* ! __LINUX_SECURITY_H */
mm/mmap.c +1 -0
@@ -26,6 +26,7 @@#include <linux/mount.h> #include <linux/mempolicy.h> #include <linux/rmap.h> +#include <linux/security.h> #include <asm/uaccess.h> #include <asm/cacheflush.h>
mm/nommu.c +1 -0
@@ -28,6 +28,7 @@#include <linux/personality.h> #include <linux/security.h> #include <linux/syscalls.h> +#include <linux/security.h> #include <asm/uaccess.h> #include <asm/tlb.h>
On 12/19/07, David Chinner <[email protected]> wrote:
Quoted 7 lines
included
security.hinmm.hand that is how I'm seeing the namespace poisoning coming fromkey.hwhen !CONFIG_KEY.Including
security.hinmm.hmeans much wider includes for pretty much the entire kernel, and it opens up namespace issues like this that never previously existed.
ACK, removing sched.h from mm.h was quite painful and security.h added it back unconditionally. As result, standalone mm.h inclusion goes from ~9K to ~16K of code after preprocessing which is quite unpleasant.
Quoted 7 lines
include/linux/mm.h +0
-1@@ -12,7 +12,6 @@#include <linux/prio_tree.h> #include <linux/debug_locks.h> #include <linux/mm_types.h> -#include <linux/security.h>
James Morris
maintainerQuoted 28 lines · David Chinner
Folks,
I just updated a git tree and started getting errors on a "
copy_keys" macro warning.The code I've been working on uses a ->copy_keys() method for copying the keys in a btree block from one place to another. I've been working on this code for a while
(http://oss.sgi.com/archives/xfs/2007-11/msg00046.html) and keep the tree I'm working in reletively up to date (lags linus by a couple of weeks at most). The update I did this afternoon gave a conflict warning with the macro ininclude/linux/key.h.Given that I'm not directly including
key.hanywhere in the XFS code, I'm getting the namespace polluted indirectly from some other include that is necessary.As it turns out, this commit from 13 days ago:
included
security.hinmm.hand that is how I'm seeing the namespace poisoning coming fromkey.hwhen !CONFIG_KEY.Including
security.hinmm.hmeans much wider includes for pretty much the entire kernel, and it opens up namespace issues like this that never previously existed.
Quoted 5 lines
The patch below (only tested for !CONFIG_KEYS && !CONFIG_SECURITY) moves
security.hinto themmap.candnommu.cfiles that need it so it doesn't end up with kernel wide scope.Comments?
The idea with this placement was to keep memory management code with other similar code, rather than pushing it into security.h, where it does not functionally belong.
Something to not also is that you can't "depend" on security.h not being included all over the place, as LSM does touch a lot of the kernel. Unecessarily including it is bad, of course.
I'm not sure I understand your namespace pollution issue, either.
In any case, I think the right solution is not to include security.h at all in mm.h, as it is only being done to get a declaration for mmap_min_addr.
How about this, instead ?
---
mm.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
include/linux/mm.h +4 -1
@@ -12,7 +12,6 @@#include <linux/prio_tree.h> #include <linux/debug_locks.h> #include <linux/mm_types.h> -#include <linux/security.h> struct mempolicy ;struct anon_vma ;
@@ -34,6 +33,10 @@ extern int sysctl_legacy_va_layout;#define sysctl_legacy_va_layout 0#endif +#ifdef CONFIG_SECURITY +extern unsigned long mmap_min_addr; +#endif +#include <asm/page.h> #include <asm/pgtable.h> #include <asm/processor.h>
Signature · 2 lines
James Morris <[email protected]>
Quoted 36 lines · James Morris
Quoted 28 lines · David Chinner
Folks,
I just updated a git tree and started getting errors on a "
copy_keys" macro warning.The code I've been working on uses a ->copy_keys() method for copying the keys in a btree block from one place to another. I've been working on this code for a while
(http://oss.sgi.com/archives/xfs/2007-11/msg00046.html) and keep the tree I'm working in reletively up to date (lags linus by a couple of weeks at most). The update I did this afternoon gave a conflict warning with the macro ininclude/linux/key.h.Given that I'm not directly including
key.hanywhere in the XFS code, I'm getting the namespace polluted indirectly from some other include that is necessary.As it turns out, this commit from 13 days ago:
included
security.hinmm.hand that is how I'm seeing the namespace poisoning coming fromkey.hwhen !CONFIG_KEY.Including
security.hinmm.hmeans much wider includes for pretty much the entire kernel, and it opens up namespace issues like this that never previously existed.Quoted 5 lines
The patch below (only tested for !CONFIG_KEYS && !CONFIG_SECURITY) moves
security.hinto themmap.candnommu.cfiles that need it so it doesn't end up with kernel wide scope.Comments?
Quoted 7 lines
The idea with this placement was to keep memory management code with other similar code, rather than pushing it into
security.h, where it does not functionally belong.Something to not also is that you can't "depend" on
security.hnot being included all over the place, as LSM does touch a lot of the kernel. Unecessarily including it is bad, of course.
Which is what including it in mm.h does. It also pull sin a lot of other headers files as has already been noted.
Quoted 1 line
I'm not sure I understand your namespace pollution issue, either.
doing this globally:
#ifdef CONFIG_SOMETHING
extern int some_common_name(int a, int b, int c);
#else
#define some_common_name(a,b,c) 0
#endif
means that no-one can use some_common_name *anywhere* in the kernel. In this case, i have a completely *private* use of some_common_name and now I can't use that because the wonderful define above that now has effectively global scope because it gets included from key.h via security.h via mm.h.
Quoted 25 lines
In any case, I think the right solution is not to include
security.hat all inmm.h, as it is only being done to get a declaration formmap_min_addr.How about this, instead ?
Signed-off-by James Morris <[email protected]>---
mm.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)include/linux/mm.h +0
-1@@ -12,7 +12,6 @@#include <linux/prio_tree.h> #include <linux/debug_locks.h> #include <linux/mm_types.h> -#include <linux/security.h> struct mempolicy ;struct anon_vma ;
include/linux/mm.h extern int sysctl_legacy_va_layout; · line 42
Quoted 10 lines
include/linux/mm.h +4
-0@@ -34,6 +33,10 @@ extern int sysctl_legacy_va_layout;#define sysctl_legacy_va_layout 0#endif +#ifdef CONFIG_SECURITY +extern unsigned long mmap_min_addr; +#endif +#include <asm/page.h> #include <asm/pgtable.h> #include <asm/processor.h>
Fine by me.
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
James Morris
maintainerQuoted 9 lines · David Chinner
Quoted 1 line
I'm not sure I understand your namespace pollution issue, either.
doing this globally:
#ifdef
CONFIG_SOMETHING
extern intsome_common_name(int a, int b, int c);
#else
#definesome_common_name(a,b,c)0
#endif
I suspect it may be useful ensure all global identifiers for the key subsystem are prefixed with key_, as 'copy_keys' does seem a little generic.
Quoted 9 lines
include/linux/mm.h extern int sysctl_legacy_va_layout; · line 42
Quoted 7 lines
include/linux/mm.h +4
-0+#ifdef CONFIG_SECURITY +extern unsigned long mmap_min_addr; +#endif +#include <asm/page.h> #include <asm/pgtable.h> #include <asm/processor.h> Fine by me.
I'll queue it for -mm & 2.6.25.
- James
Signature · 2 lines
James Morris <[email protected]>
Andrew Morton
maintainerQuoted 11 lines · On Thu, 20 Dec 2007 15:11:40 +1100 (EST) James Morris <[email protected]> wrote:
Quoted 9 lines
include/linux/mm.h extern int sysctl_legacy_va_layout; · line 42
Quoted 7 lines
include/linux/mm.h +4
-0+#ifdef CONFIG_SECURITY +extern unsigned long mmap_min_addr; +#endif +#include <asm/page.h> #include <asm/pgtable.h> #include <asm/processor.h> Fine by me.
I'll queue it for -mm & 2.6.25.
I don't think we need the ifdef. If someone incorrectly refers to this then they'll get a link-time error rather than a compile-time error (bad). But we lose an ifdef (good).
James Morris
maintainerQuoted 12 lines · Andrew Morton
Quoted 10 lines · On Thu, 20 Dec 2007 15:11:40 +1100 (EST) James Morris <[email protected]> wrote:
Quoted 9 lines
include/linux/mm.h extern int sysctl_legacy_va_layout; · line 42
Quoted 7 lines
include/linux/mm.h +4
-0+#ifdef CONFIG_SECURITY +extern unsigned long mmap_min_addr; +#endif +#include <asm/page.h> #include <asm/pgtable.h> #include <asm/processor.h> Fine by me.
Quoted 5 lines
Quoted 1 line
I'll queue it for -mm & 2.6.25.
I don't think we need the ifdef. If someone incorrectly refers to this then they'll get a link-time error rather than a compile-time error (bad). But we lose an ifdef (good).
Done, & rebased the git branch.
- James
Signature · 2 lines
James Morris <[email protected]>
Quoted 3 lines · James Morris
I suspect it may be useful ensure all global identifiers for the key subsystem are prefixed with
key_, as 'copy_keys' does seem a little generic.
Many of the fork helpers are called copy_xxx().
David
Eric Paris
Acked-byQuoted 46 lines · James Morris
Quoted 28 lines · David Chinner
Folks,
I just updated a git tree and started getting errors on a "
copy_keys" macro warning.The code I've been working on uses a ->copy_keys() method for copying the keys in a btree block from one place to another. I've been working on this code for a while
(http://oss.sgi.com/archives/xfs/2007-11/msg00046.html) and keep the tree I'm working in reletively up to date (lags linus by a couple of weeks at most). The update I did this afternoon gave a conflict warning with the macro ininclude/linux/key.h.Given that I'm not directly including
key.hanywhere in the XFS code, I'm getting the namespace polluted indirectly from some other include that is necessary.As it turns out, this commit from 13 days ago:
included
security.hinmm.hand that is how I'm seeing the namespace poisoning coming fromkey.hwhen !CONFIG_KEY.Including
security.hinmm.hmeans much wider includes for pretty much the entire kernel, and it opens up namespace issues like this that never previously existed.Quoted 5 lines
The patch below (only tested for !CONFIG_KEYS && !CONFIG_SECURITY) moves
security.hinto themmap.candnommu.cfiles that need it so it doesn't end up with kernel wide scope.Comments?
The idea with this placement was to keep memory management code with other similar code, rather than pushing it into
security.h, where it does not functionally belong.Something to not also is that you can't "depend" on
security.hnot being included all over the place, as LSM does touch a lot of the kernel. Unecessarily including it is bad, of course.I'm not sure I understand your namespace pollution issue, either.
Quoted 7 lines
In any case, I think the right solution is not to include
security.hat all inmm.h, as it is only being done to get a declaration formmap_min_addr.How about this, instead ?
Signed-off-by James Morris <[email protected]>
Quoted 30 lines
---
mm.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)include/linux/mm.h +4
-1@@ -12,7 +12,6 @@#include <linux/prio_tree.h> #include <linux/debug_locks.h> #include <linux/mm_types.h> -#include <linux/security.h> struct mempolicy ;struct anon_vma ;@@ -34,6 +33,10 @@ extern int sysctl_legacy_va_layout;#define sysctl_legacy_va_layout 0#endif +#ifdef CONFIG_SECURITY +extern unsigned long mmap_min_addr; +#endif +#include <asm/page.h> #include <asm/pgtable.h> #include <asm/processor.h>