Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
On Thu, Sep 13, 2018 at 8:55 AM peter enderborg wrote: > On 09/13/2018 01:11 PM, Michal Hocko wrote: > > On Thu 13-09-18 09:12:04, peter enderborg wrote: > >> On 09/13/2018 08:26 AM, Tetsuo Handa wrote: > >>> On 2018/09/13 12:02, Paul Moore wrote: > On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa > wrote: > > syzbot is hitting warning at str_read() [1] because len parameter can > > become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for > > this case. > > > > [1] > > https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 > > > > Signed-off-by: Tetsuo Handa > > Reported-by: syzbot > > > > --- > > security/selinux/ss/policydb.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/security/selinux/ss/policydb.c > > b/security/selinux/ss/policydb.c > > index e9394e7..f4eadd3 100644 > > --- a/security/selinux/ss/policydb.c > > +++ b/security/selinux/ss/policydb.c > > @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, > > void *fp, u32 len) > > if ((len == 0) || (len == (u32)-1)) > > return -EINVAL; > > > > - str = kmalloc(len + 1, flags); > > + str = kmalloc(len + 1, flags | __GFP_NOWARN); > > if (!str) > > return -ENOMEM; > Thanks for the patch. > > My eyes are starting to glaze over a bit chasing down all of the > different kmalloc() code paths trying to ensure that this always does > the right thing based on size of the allocation and the different slab > allocators ... are we sure that this will always return NULL when (len > + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator > configurations? > > >>> Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return > >>> ZERO_SIZE_PTR) due to (len == (u32)-1) check above. > >>> > >>> The only concern would be whether you want allocation failure messages. > >>> I assumed you don't need it because we are returning -ENOMEM to the > >>> caller. > >>> > >> Would it not be better with > >> > >> char *str; > >> > >> if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE)) > >> return -EINVAL; > >> > >> str = kmalloc(len + 1, flags); > >> if (!str) > >> return -ENOMEM; > > I strongly suspect that you want kvmalloc rather than kmalloc here. The > > larger the request the more likely is the allocation to fail. > > > > I am not familiar with the code but I assume this is a root only > > interface so we don't have to worry about nasty users scenario. > > > I don't think we get any big data there at all. Usually less than 32 bytes. > However this data can be in fast path so a vmalloc is not an option. > > And some of the calls are GFP_ATOMC. Based on all the comments it looks like Tetsuo's original patch is probably the best fix right now. I'm going to merge this into selinux/next. Tetsuo, thanks for the patch, and thanks to everyone else for the comments/review. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
On Thu, Sep 13, 2018 at 3:12 AM peter enderborg wrote: > On 09/13/2018 08:26 AM, Tetsuo Handa wrote: > > On 2018/09/13 12:02, Paul Moore wrote: > >> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa > >> wrote: > >>> syzbot is hitting warning at str_read() [1] because len parameter can > >>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for > >>> this case. > >>> > >>> [1] > >>> https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 > >>> > >>> Signed-off-by: Tetsuo Handa > >>> Reported-by: syzbot > >>> > >>> --- > >>> security/selinux/ss/policydb.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/security/selinux/ss/policydb.c > >>> b/security/selinux/ss/policydb.c > >>> index e9394e7..f4eadd3 100644 > >>> --- a/security/selinux/ss/policydb.c > >>> +++ b/security/selinux/ss/policydb.c > >>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void > >>> *fp, u32 len) > >>> if ((len == 0) || (len == (u32)-1)) > >>> return -EINVAL; > >>> > >>> - str = kmalloc(len + 1, flags); > >>> + str = kmalloc(len + 1, flags | __GFP_NOWARN); > >>> if (!str) > >>> return -ENOMEM; > >> Thanks for the patch. > >> > >> My eyes are starting to glaze over a bit chasing down all of the > >> different kmalloc() code paths trying to ensure that this always does > >> the right thing based on size of the allocation and the different slab > >> allocators ... are we sure that this will always return NULL when (len > >> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator > >> configurations? > >> > > Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return > > ZERO_SIZE_PTR) due to (len == (u32)-1) check above. > > > > The only concern would be whether you want allocation failure messages. > > I assumed you don't need it because we are returning -ENOMEM to the caller. > > > Would it not be better with > > char *str; > > if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE)) > return -EINVAL; > > str = kmalloc(len + 1, flags); > if (!str) > return -ENOMEM; As long as it's safe, I'd rather leave the maximum allocation limit as a kmalloc internal and let kmalloc return NULL if we try too large of an allocation. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
On Thu, Sep 13, 2018 at 2:26 AM Tetsuo Handa wrote: > On 2018/09/13 12:02, Paul Moore wrote: > > On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa > > wrote: > >> syzbot is hitting warning at str_read() [1] because len parameter can > >> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for > >> this case. > >> > >> [1] > >> https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 > >> > >> Signed-off-by: Tetsuo Handa > >> Reported-by: syzbot > >> --- > >> security/selinux/ss/policydb.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/security/selinux/ss/policydb.c > >> b/security/selinux/ss/policydb.c > >> index e9394e7..f4eadd3 100644 > >> --- a/security/selinux/ss/policydb.c > >> +++ b/security/selinux/ss/policydb.c > >> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void > >> *fp, u32 len) > >> if ((len == 0) || (len == (u32)-1)) > >> return -EINVAL; > >> > >> - str = kmalloc(len + 1, flags); > >> + str = kmalloc(len + 1, flags | __GFP_NOWARN); > >> if (!str) > >> return -ENOMEM; > > > > Thanks for the patch. > > > > My eyes are starting to glaze over a bit chasing down all of the > > different kmalloc() code paths trying to ensure that this always does > > the right thing based on size of the allocation and the different slab > > allocators ... are we sure that this will always return NULL when (len > > + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator > > configurations? > > Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return > ZERO_SIZE_PTR) due to (len == (u32)-1) check above. > > The only concern would be whether you want allocation failure messages. > I assumed you don't need it because we are returning -ENOMEM to the caller. I'm not to worried about the failure messages, returning -ENOMEM should be sufficient in this case. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
On Thu, Sep 13, 2018 at 11:19 AM Kees Cook wrote:
> On Thu, Sep 13, 2018 at 6:16 AM, Paul Moore wrote:
> > On Thu, Sep 13, 2018 at 12:19 AM Kees Cook wrote:
> >> On Tue, Sep 11, 2018 at 9:42 AM, Casey Schaufler
> >> wrote:
> >> > Two proposed security modules require the ability to
> >> > share security blobs with existing "major" security modules.
> >> > These modules, S.A.R.A and LandLock, provide significantly
> >> > different services than SELinux, Smack or AppArmor. Using
> >> > either in conjunction with the existing modules is quite
> >> > reasonable. S.A.R.A requires access to the cred blob, while
> >> > LandLock uses the cred, file and inode blobs.
> >> >
> >> > The use of the cred, file and inode blobs has been
> >> > abstracted in preceding patches in the series. This
> >> > patch teaches the affected security modules how to access
> >> > the part of the blob set aside for their use in the case
> >> > where blobs are shared. The configuration option
> >> > CONFIG_SECURITY_STACKING identifies systems where the
> >> > blobs may be shared.
> >> >
> >> > The mechanism for selecting which security modules are
> >> > active has been changed to allow non-conflicting "major"
> >> > security modules to be used together. At this time the
> >> > TOMOYO module can safely be used with any of the others.
> >> > The two new modules would be non-conflicting as well.
> >> >
> >> > Signed-off-by: Casey Schaufler
> >> > ---
> >> > Documentation/admin-guide/LSM/index.rst | 14 +++--
> >> > include/linux/lsm_hooks.h | 2 +-
> >> > security/Kconfig| 81 +
> >> > security/apparmor/include/cred.h| 8 +++
> >> > security/apparmor/include/file.h| 9 ++-
> >> > security/apparmor/include/lib.h | 4 ++
> >> > security/apparmor/lsm.c | 8 ++-
> >> > security/security.c | 30 -
> >> > security/selinux/hooks.c| 3 +-
> >> > security/selinux/include/objsec.h | 18 +-
> >> > security/smack/smack.h | 19 +-
> >> > security/smack/smack_lsm.c | 17 +++---
> >> > security/tomoyo/common.h| 12 +++-
> >> > security/tomoyo/tomoyo.c| 3 +-
> >> > 14 files changed, 200 insertions(+), 28 deletions(-)
> >
> > ...
> >
> >> > diff --git a/security/Kconfig b/security/Kconfig
> >> > index 22f7664c4977..ed48025ae9e0 100644
> >> > --- a/security/Kconfig
> >> > +++ b/security/Kconfig
> >> > @@ -36,6 +36,28 @@ config SECURITY_WRITABLE_HOOKS
> >> > bool
> >> > default n
> >> >
> >> > +config SECURITY_STACKING
> >> > + bool "Security module stacking"
> >> > + depends on SECURITY
> >> > + help
> >> > + Allows multiple major security modules to be stacked.
> >> > + Modules are invoked in the order registered with a
> >> > + "bail on fail" policy, in which the infrastructure
> >> > + will stop processing once a denial is detected. Not
> >> > + all modules can be stacked. SELinux, Smack and AppArmor are
> >> > + known to be incompatible. User space components may
> >> > + have trouble identifying the security module providing
> >> > + data in some cases.
> >> > +
> >> > + If you select this option you will have to select which
> >> > + of the stackable modules you wish to be active. The
> >> > + "Default security module" will be ignored. The boot line
> >> > + "security=" option can be used to specify that one of
> >> > + the modules identifed for stacking should be used instead
> >> > + of the entire stack.
> >> > +
> >> > + If you are unsure how to answer this question, answer N.
> >>
> >> I don't see a good reason to make this a config. Why shouldn't this
> >> always be enabled?
> >
> > I do. From a user perspective it is sometimes difficult to determine
> > the reason behind a failed operation; its is a DAC based denial, the
> > LSM, or some other failure? Stacking additional LSMs has the
> > potential to make this worse. The boot time configuration adds to the
> > complexity.
>
> Let me try to convince you otherwise. :) The reason I think there's no
> need for this is because the only functional change here is how
> _TOMOYO_ gets stacked. And in my proposal, we can convert TOMOYO to be
> enabled/disabled like LoadPin. Given the configs I showed, stacking
> TOMOYO with the other major LSMs becomes a config (and/or boottime)
> option.
>
> The changes for TOMOYO are still needed even _with_ SECURITY_STACKING,
> and I argue that the other major LSMs remain the same. It's only
> infrastructure that has changed. So, I think having SECURITY_STACKING
> actually makes things more complex internally (all the ifdefs, weird
> enable logic) and for distros ("what's this stacking option", etc?)
None of the above deals with the user experience or support burden a
distro would
Re: [PATCH 04/10] LSM: Infrastructure management of the cred security blob
On 9/12/2018 4:53 PM, Kees Cook wrote:
> On Tue, Sep 11, 2018 at 9:41 AM, Casey Schaufler
> wrote:
>> Move management of the cred security blob out of the
>> security modules and into the security infrastructure.
>> Instead of allocating and freeing space the security
>> modules tell the infrastructure how much space they
>> require.
> There's a lot of changes here that I think deserve some longer
> discussion in the changelog. Notably, now we run _two_ cycles of init
> calls? That seems... odd. Notes below...
The first pass adds up the blob sizes. This has to be done because
some modules allocate blobs during the init phase. I have investigated
alternatives, including blobs that include information about what they
contain, but they're all significantly more complicated.
>> Some SELinux memory management debug code has been removed.
>>
>> Signed-off-by: Casey Schaufler
>> ---
>> include/linux/lsm_hooks.h | 14
>> kernel/cred.c | 13
>> security/Kconfig | 11
>> security/apparmor/domain.c| 2 +-
>> security/apparmor/include/cred.h | 16 -
>> security/apparmor/lsm.c | 28 ++--
>> security/apparmor/task.c | 6 +-
>> security/security.c | 106 +-
>> security/selinux/hooks.c | 63 +-
>> security/selinux/include/objsec.h | 4 ++
>> security/selinux/selinuxfs.c | 1 +
>> security/smack/smack.h| 1 +
>> security/smack/smack_lsm.c| 85 +---
>> security/tomoyo/common.h | 21 +-
>> security/tomoyo/domain.c | 4 +-
>> security/tomoyo/securityfs_if.c | 15 +++--
>> security/tomoyo/tomoyo.c | 56 +---
>> 17 files changed, 303 insertions(+), 143 deletions(-)
>>
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index 97a020c616ad..0bef312efd45 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -2024,6 +2024,13 @@ struct security_hook_list {
>> char*lsm;
>> } __randomize_layout;
>>
>> +/*
>> + * Security blob size or offset data.
>> + */
>> +struct lsm_blob_sizes {
>> + int lbs_cred;
>> +};
>> +
>> /*
>> * Initializing a security_hook_list structure takes
>> * up a lot of space in a source file. This macro takes
>> @@ -2036,6 +2043,7 @@ struct security_hook_list {
>> extern struct security_hook_heads security_hook_heads;
>> extern char *lsm_names;
>>
>> +extern void security_add_blobs(struct lsm_blob_sizes *needed);
>> extern void security_add_hooks(struct security_hook_list *hooks, int count,
>> char *lsm);
>>
>> @@ -2082,4 +2090,10 @@ void __init loadpin_add_hooks(void);
>> static inline void loadpin_add_hooks(void) { };
>> #endif
>>
>> +extern int lsm_cred_alloc(struct cred *cred, gfp_t gfp);
>> +
>> +#ifdef CONFIG_SECURITY
>> +void lsm_early_cred(struct cred *cred);
>> +#endif
>> +
>> #endif /* ! __LINUX_LSM_HOOKS_H */
>> diff --git a/kernel/cred.c b/kernel/cred.c
>> index ecf03657e71c..fa2061ee4955 100644
>> --- a/kernel/cred.c
>> +++ b/kernel/cred.c
>> @@ -704,19 +704,6 @@ bool creds_are_invalid(const struct cred *cred)
>> {
>> if (cred->magic != CRED_MAGIC)
>> return true;
>> -#ifdef CONFIG_SECURITY_SELINUX
>> - /*
>> -* cred->security == NULL if security_cred_alloc_blank() or
>> -* security_prepare_creds() returned an error.
>> -*/
>> - if (selinux_is_enabled() && cred->security) {
>> - if ((unsigned long) cred->security < PAGE_SIZE)
>> - return true;
>> - if ((*(u32 *)cred->security & 0xff00) ==
>> - (POISON_FREE << 24 | POISON_FREE << 16 | POISON_FREE <<
>> 8))
>> - return true;
> These aren't unreasonable checks -- can we add them back in later?
> (They don't need to be selinux specific, in fact: the LSM could do the
> poison...)
I had asked the maintainers about the value of these checks, and
they said that they were primarily there for debugging during the
original cred breakout development. I'd have not problem making them
infrastructure managed if there's a strong desire to keep them.
>> [...]
>> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
>> index 08c88de0ffda..726910bba84b 100644
>> --- a/security/apparmor/domain.c
>> +++ b/security/apparmor/domain.c
>> @@ -975,7 +975,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>> }
>> aa_put_label(cred_label(bprm->cred));
>> /* transfer reference, released when cred is freed */
>> - cred_label(bprm->cred) = new;
>> + set_cred_label(bprm->cred, new);
>>
>> done:
>> aa_put_label(label);
>> diff --git a/security/apparmor/include/cred.h
>> b/security/apparmor/include/cred.h
>> index e287b7d0d4be..a90eae76d7c1 100644
Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
On Thu, Sep 13, 2018 at 6:16 AM, Paul Moore wrote:
> On Thu, Sep 13, 2018 at 12:19 AM Kees Cook wrote:
>> On Tue, Sep 11, 2018 at 9:42 AM, Casey Schaufler
>> wrote:
>> > Two proposed security modules require the ability to
>> > share security blobs with existing "major" security modules.
>> > These modules, S.A.R.A and LandLock, provide significantly
>> > different services than SELinux, Smack or AppArmor. Using
>> > either in conjunction with the existing modules is quite
>> > reasonable. S.A.R.A requires access to the cred blob, while
>> > LandLock uses the cred, file and inode blobs.
>> >
>> > The use of the cred, file and inode blobs has been
>> > abstracted in preceding patches in the series. This
>> > patch teaches the affected security modules how to access
>> > the part of the blob set aside for their use in the case
>> > where blobs are shared. The configuration option
>> > CONFIG_SECURITY_STACKING identifies systems where the
>> > blobs may be shared.
>> >
>> > The mechanism for selecting which security modules are
>> > active has been changed to allow non-conflicting "major"
>> > security modules to be used together. At this time the
>> > TOMOYO module can safely be used with any of the others.
>> > The two new modules would be non-conflicting as well.
>> >
>> > Signed-off-by: Casey Schaufler
>> > ---
>> > Documentation/admin-guide/LSM/index.rst | 14 +++--
>> > include/linux/lsm_hooks.h | 2 +-
>> > security/Kconfig| 81 +
>> > security/apparmor/include/cred.h| 8 +++
>> > security/apparmor/include/file.h| 9 ++-
>> > security/apparmor/include/lib.h | 4 ++
>> > security/apparmor/lsm.c | 8 ++-
>> > security/security.c | 30 -
>> > security/selinux/hooks.c| 3 +-
>> > security/selinux/include/objsec.h | 18 +-
>> > security/smack/smack.h | 19 +-
>> > security/smack/smack_lsm.c | 17 +++---
>> > security/tomoyo/common.h| 12 +++-
>> > security/tomoyo/tomoyo.c| 3 +-
>> > 14 files changed, 200 insertions(+), 28 deletions(-)
>
> ...
>
>> > diff --git a/security/Kconfig b/security/Kconfig
>> > index 22f7664c4977..ed48025ae9e0 100644
>> > --- a/security/Kconfig
>> > +++ b/security/Kconfig
>> > @@ -36,6 +36,28 @@ config SECURITY_WRITABLE_HOOKS
>> > bool
>> > default n
>> >
>> > +config SECURITY_STACKING
>> > + bool "Security module stacking"
>> > + depends on SECURITY
>> > + help
>> > + Allows multiple major security modules to be stacked.
>> > + Modules are invoked in the order registered with a
>> > + "bail on fail" policy, in which the infrastructure
>> > + will stop processing once a denial is detected. Not
>> > + all modules can be stacked. SELinux, Smack and AppArmor are
>> > + known to be incompatible. User space components may
>> > + have trouble identifying the security module providing
>> > + data in some cases.
>> > +
>> > + If you select this option you will have to select which
>> > + of the stackable modules you wish to be active. The
>> > + "Default security module" will be ignored. The boot line
>> > + "security=" option can be used to specify that one of
>> > + the modules identifed for stacking should be used instead
>> > + of the entire stack.
>> > +
>> > + If you are unsure how to answer this question, answer N.
>>
>> I don't see a good reason to make this a config. Why shouldn't this
>> always be enabled?
>
> I do. From a user perspective it is sometimes difficult to determine
> the reason behind a failed operation; its is a DAC based denial, the
> LSM, or some other failure? Stacking additional LSMs has the
> potential to make this worse. The boot time configuration adds to the
> complexity.
Let me try to convince you otherwise. :) The reason I think there's no
need for this is because the only functional change here is how
_TOMOYO_ gets stacked. And in my proposal, we can convert TOMOYO to be
enabled/disabled like LoadPin. Given the configs I showed, stacking
TOMOYO with the other major LSMs becomes a config (and/or boottime)
option.
The changes for TOMOYO are still needed even _with_ SECURITY_STACKING,
and I argue that the other major LSMs remain the same. It's only
infrastructure that has changed. So, I think having SECURITY_STACKING
actually makes things more complex internally (all the ifdefs, weird
enable logic) and for distros ("what's this stacking option", etc?)
> I think we should leave this decision to the individual distros so
> that they can make their own decision on LSM stacking based on the
> savviness of their user base and the quality of their support
> infrastructure.
If we reach the "extreme" stacking case, then yes, I want to make sure
we've got something that makes
Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
On Thu, Sep 13, 2018 at 2:55 PM, peter enderborg wrote: >> syzbot is hitting warning at str_read() [1] because len parameter can >> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for >> this case. >> >> [1] >> https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 >> >> Signed-off-by: Tetsuo Handa >> Reported-by: syzbot >> >> --- >> security/selinux/ss/policydb.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/security/selinux/ss/policydb.c >> b/security/selinux/ss/policydb.c >> index e9394e7..f4eadd3 100644 >> --- a/security/selinux/ss/policydb.c >> +++ b/security/selinux/ss/policydb.c >> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void >> *fp, u32 len) >> if ((len == 0) || (len == (u32)-1)) >> return -EINVAL; >> >> - str = kmalloc(len + 1, flags); >> + str = kmalloc(len + 1, flags | __GFP_NOWARN); >> if (!str) >> return -ENOMEM; > Thanks for the patch. > > My eyes are starting to glaze over a bit chasing down all of the > different kmalloc() code paths trying to ensure that this always does > the right thing based on size of the allocation and the different slab > allocators ... are we sure that this will always return NULL when (len > + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator > configurations? > Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return ZERO_SIZE_PTR) due to (len == (u32)-1) check above. The only concern would be whether you want allocation failure messages. I assumed you don't need it because we are returning -ENOMEM to the caller. >>> Would it not be better with >>> >>> char *str; >>> >>> if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE)) >>> return -EINVAL; >>> >>> str = kmalloc(len + 1, flags); >>> if (!str) >>> return -ENOMEM; >> I strongly suspect that you want kvmalloc rather than kmalloc here. The >> larger the request the more likely is the allocation to fail. >> >> I am not familiar with the code but I assume this is a root only >> interface so we don't have to worry about nasty users scenario. >> > I don't think we get any big data there at all. Usually less than 32 bytes. > However this data can be in fast path so a vmalloc is not an option. > > And some of the calls are GFP_ATOMC. Then another option is to introduce reasonable application-specific limit and not rely on kmalloc-anything at all. We did this for some instances of this warning too. One advantage of it is that it prevents users from doing silly things (or maybe will discover bugs in user-space code better, why are they asking for megs here?). Another advantage is that what works on one version of kernel will continue to work on another version of kernel. Today it's possible that a policy works on one kernel with 4MB kmalloc limit, but breaks on another with 2MB limit. Ideally exact value of KMALLOC_MAX_SIZE does not affect anything in user-space. ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
On 09/13/2018 01:11 PM, Michal Hocko wrote: > On Thu 13-09-18 09:12:04, peter enderborg wrote: >> On 09/13/2018 08:26 AM, Tetsuo Handa wrote: >>> On 2018/09/13 12:02, Paul Moore wrote: On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa wrote: > syzbot is hitting warning at str_read() [1] because len parameter can > become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for > this case. > > [1] > https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 > > Signed-off-by: Tetsuo Handa > Reported-by: syzbot > > --- > security/selinux/ss/policydb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/selinux/ss/policydb.c > b/security/selinux/ss/policydb.c > index e9394e7..f4eadd3 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void > *fp, u32 len) > if ((len == 0) || (len == (u32)-1)) > return -EINVAL; > > - str = kmalloc(len + 1, flags); > + str = kmalloc(len + 1, flags | __GFP_NOWARN); > if (!str) > return -ENOMEM; Thanks for the patch. My eyes are starting to glaze over a bit chasing down all of the different kmalloc() code paths trying to ensure that this always does the right thing based on size of the allocation and the different slab allocators ... are we sure that this will always return NULL when (len + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator configurations? >>> Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return >>> ZERO_SIZE_PTR) due to (len == (u32)-1) check above. >>> >>> The only concern would be whether you want allocation failure messages. >>> I assumed you don't need it because we are returning -ENOMEM to the caller. >>> >> Would it not be better with >> >> char *str; >> >> if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE)) >> return -EINVAL; >> >> str = kmalloc(len + 1, flags); >> if (!str) >> return -ENOMEM; > I strongly suspect that you want kvmalloc rather than kmalloc here. The > larger the request the more likely is the allocation to fail. > > I am not familiar with the code but I assume this is a root only > interface so we don't have to worry about nasty users scenario. > I don't think we get any big data there at all. Usually less than 32 bytes. However this data can be in fast path so a vmalloc is not an option. And some of the calls are GFP_ATOMC. ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
On Thu, Sep 13, 2018 at 12:19 AM Kees Cook wrote: > On Tue, Sep 11, 2018 at 9:42 AM, Casey Schaufler > wrote: > > Two proposed security modules require the ability to > > share security blobs with existing "major" security modules. > > These modules, S.A.R.A and LandLock, provide significantly > > different services than SELinux, Smack or AppArmor. Using > > either in conjunction with the existing modules is quite > > reasonable. S.A.R.A requires access to the cred blob, while > > LandLock uses the cred, file and inode blobs. > > > > The use of the cred, file and inode blobs has been > > abstracted in preceding patches in the series. This > > patch teaches the affected security modules how to access > > the part of the blob set aside for their use in the case > > where blobs are shared. The configuration option > > CONFIG_SECURITY_STACKING identifies systems where the > > blobs may be shared. > > > > The mechanism for selecting which security modules are > > active has been changed to allow non-conflicting "major" > > security modules to be used together. At this time the > > TOMOYO module can safely be used with any of the others. > > The two new modules would be non-conflicting as well. > > > > Signed-off-by: Casey Schaufler > > --- > > Documentation/admin-guide/LSM/index.rst | 14 +++-- > > include/linux/lsm_hooks.h | 2 +- > > security/Kconfig| 81 + > > security/apparmor/include/cred.h| 8 +++ > > security/apparmor/include/file.h| 9 ++- > > security/apparmor/include/lib.h | 4 ++ > > security/apparmor/lsm.c | 8 ++- > > security/security.c | 30 - > > security/selinux/hooks.c| 3 +- > > security/selinux/include/objsec.h | 18 +- > > security/smack/smack.h | 19 +- > > security/smack/smack_lsm.c | 17 +++--- > > security/tomoyo/common.h| 12 +++- > > security/tomoyo/tomoyo.c| 3 +- > > 14 files changed, 200 insertions(+), 28 deletions(-) ... > > diff --git a/security/Kconfig b/security/Kconfig > > index 22f7664c4977..ed48025ae9e0 100644 > > --- a/security/Kconfig > > +++ b/security/Kconfig > > @@ -36,6 +36,28 @@ config SECURITY_WRITABLE_HOOKS > > bool > > default n > > > > +config SECURITY_STACKING > > + bool "Security module stacking" > > + depends on SECURITY > > + help > > + Allows multiple major security modules to be stacked. > > + Modules are invoked in the order registered with a > > + "bail on fail" policy, in which the infrastructure > > + will stop processing once a denial is detected. Not > > + all modules can be stacked. SELinux, Smack and AppArmor are > > + known to be incompatible. User space components may > > + have trouble identifying the security module providing > > + data in some cases. > > + > > + If you select this option you will have to select which > > + of the stackable modules you wish to be active. The > > + "Default security module" will be ignored. The boot line > > + "security=" option can be used to specify that one of > > + the modules identifed for stacking should be used instead > > + of the entire stack. > > + > > + If you are unsure how to answer this question, answer N. > > I don't see a good reason to make this a config. Why shouldn't this > always be enabled? I do. From a user perspective it is sometimes difficult to determine the reason behind a failed operation; its is a DAC based denial, the LSM, or some other failure? Stacking additional LSMs has the potential to make this worse. The boot time configuration adds to the complexity. I think we should leave this decision to the individual distros so that they can make their own decision on LSM stacking based on the savviness of their user base and the quality of their support infrastructure. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
On Thu 13-09-18 09:12:04, peter enderborg wrote: > On 09/13/2018 08:26 AM, Tetsuo Handa wrote: > > On 2018/09/13 12:02, Paul Moore wrote: > >> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa > >> wrote: > >>> syzbot is hitting warning at str_read() [1] because len parameter can > >>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for > >>> this case. > >>> > >>> [1] > >>> https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 > >>> > >>> Signed-off-by: Tetsuo Handa > >>> Reported-by: syzbot > >>> > >>> --- > >>> security/selinux/ss/policydb.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/security/selinux/ss/policydb.c > >>> b/security/selinux/ss/policydb.c > >>> index e9394e7..f4eadd3 100644 > >>> --- a/security/selinux/ss/policydb.c > >>> +++ b/security/selinux/ss/policydb.c > >>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void > >>> *fp, u32 len) > >>> if ((len == 0) || (len == (u32)-1)) > >>> return -EINVAL; > >>> > >>> - str = kmalloc(len + 1, flags); > >>> + str = kmalloc(len + 1, flags | __GFP_NOWARN); > >>> if (!str) > >>> return -ENOMEM; > >> Thanks for the patch. > >> > >> My eyes are starting to glaze over a bit chasing down all of the > >> different kmalloc() code paths trying to ensure that this always does > >> the right thing based on size of the allocation and the different slab > >> allocators ... are we sure that this will always return NULL when (len > >> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator > >> configurations? > >> > > Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return > > ZERO_SIZE_PTR) due to (len == (u32)-1) check above. > > > > The only concern would be whether you want allocation failure messages. > > I assumed you don't need it because we are returning -ENOMEM to the caller. > > > Would it not be better with > > char *str; > > if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE)) > return -EINVAL; > > str = kmalloc(len + 1, flags); > if (!str) > return -ENOMEM; I strongly suspect that you want kvmalloc rather than kmalloc here. The larger the request the more likely is the allocation to fail. I am not familiar with the code but I assume this is a root only interface so we don't have to worry about nasty users scenario. -- Michal Hocko SUSE Labs ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 06/10] LSM: Infrastructure management of the file security blob
On Tue, Sep 11, 2018 at 9:42 AM, Casey Schaufler wrote:
> Move management of the file->f_security blob out of the
> individual security modules and into the infrastructure.
> The modules no longer allocate or free the data, instead
> they tell the infrastructure how much space they require.
>
> Signed-off-by: Casey Schaufler
> ---
> include/linux/lsm_hooks.h | 1 +
> security/apparmor/lsm.c| 19 +++---
> security/security.c| 54 +++---
> security/selinux/hooks.c | 25 ++
> security/smack/smack.h | 5
> security/smack/smack_lsm.c | 26 +++---
> 6 files changed, 78 insertions(+), 52 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 0bef312efd45..167ffbd4d0c0 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2029,6 +2029,7 @@ struct security_hook_list {
> */
> struct lsm_blob_sizes {
> int lbs_cred;
> + int lbs_file;
> };
>
> /*
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index c2566aaa138e..15716b6ff860 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -431,21 +431,21 @@ static int apparmor_file_open(struct file *file)
>
> static int apparmor_file_alloc_security(struct file *file)
> {
> - int error = 0;
> -
> - /* freed by apparmor_file_free_security */
> + struct aa_file_ctx *ctx = file_ctx(file);
> struct aa_label *label = begin_current_label_crit_section();
> - file->f_security = aa_alloc_file_ctx(label, GFP_KERNEL);
> - if (!file_ctx(file))
> - error = -ENOMEM;
> - end_current_label_crit_section(label);
>
> - return error;
> + spin_lock_init(>lock);
> + rcu_assign_pointer(ctx->label, aa_get_label(label));
> + end_current_label_crit_section(label);
> + return 0;
> }
>
> static void apparmor_file_free_security(struct file *file)
> {
> - aa_free_file_ctx(file_ctx(file));
> + struct aa_file_ctx *ctx = file_ctx(file);
> +
> + if (ctx)
> + aa_put_label(rcu_access_pointer(ctx->label));
> }
>
> static int common_file_perm(const char *op, struct file *file, u32 mask)
> @@ -1131,6 +1131,7 @@ static void apparmor_sock_graft(struct sock *sk, struct
> socket *parent)
> */
> struct lsm_blob_sizes apparmor_blob_sizes = {
> .lbs_cred = sizeof(struct aa_task_ctx *),
> + .lbs_file = sizeof(struct aa_file_ctx),
> };
>
> static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
> diff --git a/security/security.c b/security/security.c
> index ff7df14f6db1..5430cae73cf6 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -40,6 +40,8 @@
> struct security_hook_heads security_hook_heads __lsm_ro_after_init;
> static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
>
> +static struct kmem_cache *lsm_file_cache;
> +
> char *lsm_names;
> static struct lsm_blob_sizes blob_sizes;
>
> @@ -92,6 +94,13 @@ int __init security_init(void)
> */
> do_security_initcalls();
>
> + /*
> +* Create any kmem_caches needed for blobs
> +*/
> + if (blob_sizes.lbs_file)
> + lsm_file_cache = kmem_cache_create("lsm_file_cache",
> + blob_sizes.lbs_file, 0,
> + SLAB_PANIC, NULL);
> /*
> * The second call to a module specific init function
> * adds hooks to the hook lists and does any other early
> @@ -101,6 +110,7 @@ int __init security_init(void)
>
> #ifdef CONFIG_SECURITY_LSM_DEBUG
> pr_info("LSM: cred blob size = %d\n", blob_sizes.lbs_cred);
> + pr_info("LSM: file blob size = %d\n", blob_sizes.lbs_file);
> #endif
>
> return 0;
> @@ -277,6 +287,28 @@ static void __init lsm_set_size(int *need, int *lbs)
> void __init security_add_blobs(struct lsm_blob_sizes *needed)
> {
> lsm_set_size(>lbs_cred, _sizes.lbs_cred);
> + lsm_set_size(>lbs_file, _sizes.lbs_file);
> +}
> +
> +/**
> + * lsm_file_alloc - allocate a composite file blob
> + * @file: the file that needs a blob
> + *
> + * Allocate the file blob for all the modules
> + *
> + * Returns 0, or -ENOMEM if memory can't be allocated.
> + */
> +int lsm_file_alloc(struct file *file)
> +{
> + if (!lsm_file_cache) {
> + file->f_security = NULL;
> + return 0;
> + }
> +
> + file->f_security = kmem_cache_zalloc(lsm_file_cache, GFP_KERNEL);
> + if (file->f_security == NULL)
> + return -ENOMEM;
> + return 0;
> }
>
> /*
> @@ -962,12 +994,28 @@ int security_file_permission(struct file *file, int
> mask)
>
> int security_file_alloc(struct file *file)
> {
> - return call_int_hook(file_alloc_security, 0, file);
> + int rc = lsm_file_alloc(file);
> +
> + if (rc)
> +
Re: [PATCH 08/10] Smack: Abstract use of inode security blob
On Tue, Sep 11, 2018 at 9:42 AM, Casey Schaufler wrote: > Don't use the inode->i_security pointer directly. > Provide a helper function that provides the security blob pointer. > > Signed-off-by: Casey Schaufler Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
On Tue, Sep 11, 2018 at 9:42 AM, Casey Schaufler wrote: > Two proposed security modules require the ability to > share security blobs with existing "major" security modules. > These modules, S.A.R.A and LandLock, provide significantly > different services than SELinux, Smack or AppArmor. Using > either in conjunction with the existing modules is quite > reasonable. S.A.R.A requires access to the cred blob, while > LandLock uses the cred, file and inode blobs. > > The use of the cred, file and inode blobs has been > abstracted in preceding patches in the series. This > patch teaches the affected security modules how to access > the part of the blob set aside for their use in the case > where blobs are shared. The configuration option > CONFIG_SECURITY_STACKING identifies systems where the > blobs may be shared. > > The mechanism for selecting which security modules are > active has been changed to allow non-conflicting "major" > security modules to be used together. At this time the > TOMOYO module can safely be used with any of the others. > The two new modules would be non-conflicting as well. > > Signed-off-by: Casey Schaufler > --- > Documentation/admin-guide/LSM/index.rst | 14 +++-- > include/linux/lsm_hooks.h | 2 +- > security/Kconfig| 81 + > security/apparmor/include/cred.h| 8 +++ > security/apparmor/include/file.h| 9 ++- > security/apparmor/include/lib.h | 4 ++ > security/apparmor/lsm.c | 8 ++- > security/security.c | 30 - > security/selinux/hooks.c| 3 +- > security/selinux/include/objsec.h | 18 +- > security/smack/smack.h | 19 +- > security/smack/smack_lsm.c | 17 +++--- > security/tomoyo/common.h| 12 +++- > security/tomoyo/tomoyo.c| 3 +- > 14 files changed, 200 insertions(+), 28 deletions(-) > > diff --git a/Documentation/admin-guide/LSM/index.rst > b/Documentation/admin-guide/LSM/index.rst > index 9842e21afd4a..d3d8af174042 100644 > --- a/Documentation/admin-guide/LSM/index.rst > +++ b/Documentation/admin-guide/LSM/index.rst > @@ -17,10 +17,16 @@ MAC extensions, other extensions can be built using the > LSM to provide > specific changes to system operation when these tweaks are not available > in the core functionality of Linux itself. > > -The Linux capabilities modules will always be included. This may be > -followed by any number of "minor" modules and at most one "major" module. > -For more details on capabilities, see ``capabilities(7)`` in the Linux > -man-pages project. > +The Linux capabilities modules will always be included. For more details > +on capabilities, see ``capabilities(7)`` in the Linux man-pages project. > + > +Security modules that do not use the security data blobs maintained > +by the LSM infrastructure are considered "minor" modules. These may be > +included at compile time and stacked explicitly. Security modules that > +use the LSM maintained security blobs are considered "major" modules. > +These may only be stacked if the CONFIG_LSM_STACKED configuration > +option is used. If this is chosen all of the security modules selected > +will be used. > > A list of the active security modules can be found by reading > ``/sys/kernel/security/lsm``. This is a comma separated list, and > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 416b20c3795b..dddcced54fea 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -2079,7 +2079,7 @@ static inline void security_delete_hooks(struct > security_hook_list *hooks, > #define __lsm_ro_after_init__ro_after_init > #endif /* CONFIG_SECURITY_WRITABLE_HOOKS */ > > -extern int __init security_module_enable(const char *module); > +extern bool __init security_module_enable(const char *lsm, const bool > stacked); > extern void __init capability_add_hooks(void); > #ifdef CONFIG_SECURITY_YAMA > extern void __init yama_add_hooks(void); > diff --git a/security/Kconfig b/security/Kconfig > index 22f7664c4977..ed48025ae9e0 100644 > --- a/security/Kconfig > +++ b/security/Kconfig > @@ -36,6 +36,28 @@ config SECURITY_WRITABLE_HOOKS > bool > default n > > +config SECURITY_STACKING > + bool "Security module stacking" > + depends on SECURITY > + help > + Allows multiple major security modules to be stacked. > + Modules are invoked in the order registered with a > + "bail on fail" policy, in which the infrastructure > + will stop processing once a denial is detected. Not > + all modules can be stacked. SELinux, Smack and AppArmor are > + known to be incompatible. User space components may > + have trouble identifying the security module providing > + data in some cases. > + > + If you select this option you will have to select which > +
Re: [PATCH 07/10] SELinux: Abstract use of inode security blob
On Tue, Sep 11, 2018 at 9:42 AM, Casey Schaufler wrote: > Don't use the inode->i_security pointer directly. > Provide a helper function that provides the security blob pointer. > > Signed-off-by: Casey Schaufler Happily mechanical! :) Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 05/10] SELinux: Abstract use of file security blob
On Tue, Sep 11, 2018 at 9:41 AM, Casey Schaufler wrote: > Don't use the file->f_security pointer directly. > Provide a helper function that provides the security blob pointer. > > Signed-off-by: Casey Schaufler Seems delightfully mechanical. Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
On Thu, Sep 13, 2018 at 5:02 AM, Paul Moore wrote: > On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa > wrote: >> syzbot is hitting warning at str_read() [1] because len parameter can >> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for >> this case. >> >> [1] >> https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 >> >> Signed-off-by: Tetsuo Handa >> Reported-by: syzbot >> --- >> security/selinux/ss/policydb.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c >> index e9394e7..f4eadd3 100644 >> --- a/security/selinux/ss/policydb.c >> +++ b/security/selinux/ss/policydb.c >> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void >> *fp, u32 len) >> if ((len == 0) || (len == (u32)-1)) >> return -EINVAL; >> >> - str = kmalloc(len + 1, flags); >> + str = kmalloc(len + 1, flags | __GFP_NOWARN); >> if (!str) >> return -ENOMEM; > > Thanks for the patch. > > My eyes are starting to glaze over a bit chasing down all of the > different kmalloc() code paths trying to ensure that this always does > the right thing based on size of the allocation and the different slab > allocators ... are we sure that this will always return NULL when (len > + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator > configurations? Yes, it's the blessed way to do it. We have lots of similar cases: https://elixir.bootlin.com/linux/v4.19-rc3/ident/__GFP_NOWARN ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
On 2018/09/13 12:02, Paul Moore wrote: > On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa > wrote: >> syzbot is hitting warning at str_read() [1] because len parameter can >> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for >> this case. >> >> [1] >> https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 >> >> Signed-off-by: Tetsuo Handa >> Reported-by: syzbot >> --- >> security/selinux/ss/policydb.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c >> index e9394e7..f4eadd3 100644 >> --- a/security/selinux/ss/policydb.c >> +++ b/security/selinux/ss/policydb.c >> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void >> *fp, u32 len) >> if ((len == 0) || (len == (u32)-1)) >> return -EINVAL; >> >> - str = kmalloc(len + 1, flags); >> + str = kmalloc(len + 1, flags | __GFP_NOWARN); >> if (!str) >> return -ENOMEM; > > Thanks for the patch. > > My eyes are starting to glaze over a bit chasing down all of the > different kmalloc() code paths trying to ensure that this always does > the right thing based on size of the allocation and the different slab > allocators ... are we sure that this will always return NULL when (len > + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator > configurations? > Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return ZERO_SIZE_PTR) due to (len == (u32)-1) check above. The only concern would be whether you want allocation failure messages. I assumed you don't need it because we are returning -ENOMEM to the caller. ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 09/10] LSM: Infrastructure management of the inode security
On Tue, Sep 11, 2018 at 9:42 AM, Casey Schaufler wrote:
> Move management of the inode->i_security blob out
> of the individual security modules and into the security
> infrastructure. Instead of allocating the blobs from within
> the modules the modules tell the infrastructure how much
> space is required, and the space is allocated there.
>
> Signed-off-by: Casey Schaufler
> ---
> include/linux/lsm_hooks.h | 3 ++
> security/security.c | 83 ++-
> security/selinux/hooks.c | 32 +---
> security/selinux/include/objsec.h | 5 +-
> security/smack/smack_lsm.c| 70 --
> 5 files changed, 98 insertions(+), 95 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 167ffbd4d0c0..416b20c3795b 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2030,6 +2030,7 @@ struct security_hook_list {
> struct lsm_blob_sizes {
> int lbs_cred;
> int lbs_file;
> + int lbs_inode;
> };
>
> /*
> @@ -2092,9 +2093,11 @@ static inline void loadpin_add_hooks(void) { };
> #endif
>
> extern int lsm_cred_alloc(struct cred *cred, gfp_t gfp);
> +extern int lsm_inode_alloc(struct inode *inode);
>
> #ifdef CONFIG_SECURITY
> void lsm_early_cred(struct cred *cred);
> +void lsm_early_inode(struct inode *inode);
> #endif
>
> #endif /* ! __LINUX_LSM_HOOKS_H */
> diff --git a/security/security.c b/security/security.c
> index 5430cae73cf6..2501cdcbebff 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -41,6 +41,7 @@ struct security_hook_heads security_hook_heads
> __lsm_ro_after_init;
> static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
>
> static struct kmem_cache *lsm_file_cache;
> +static struct kmem_cache *lsm_inode_cache;
>
> char *lsm_names;
> static struct lsm_blob_sizes blob_sizes;
> @@ -101,6 +102,10 @@ int __init security_init(void)
> lsm_file_cache = kmem_cache_create("lsm_file_cache",
>blob_sizes.lbs_file, 0,
>SLAB_PANIC, NULL);
> + if (blob_sizes.lbs_inode)
> + lsm_inode_cache = kmem_cache_create("lsm_inode_cache",
> + blob_sizes.lbs_inode, 0,
> + SLAB_PANIC, NULL);
> /*
> * The second call to a module specific init function
> * adds hooks to the hook lists and does any other early
> @@ -111,6 +116,7 @@ int __init security_init(void)
> #ifdef CONFIG_SECURITY_LSM_DEBUG
> pr_info("LSM: cred blob size = %d\n", blob_sizes.lbs_cred);
> pr_info("LSM: file blob size = %d\n", blob_sizes.lbs_file);
> + pr_info("LSM: inode blob size = %d\n", blob_sizes.lbs_inode);
> #endif
>
> return 0;
> @@ -288,6 +294,13 @@ void __init security_add_blobs(struct lsm_blob_sizes
> *needed)
> {
> lsm_set_size(>lbs_cred, _sizes.lbs_cred);
> lsm_set_size(>lbs_file, _sizes.lbs_file);
> + /*
> +* The inode blob gets an rcu_head in addition to
> +* what the modules might need.
> +*/
> + if (needed->lbs_inode && blob_sizes.lbs_inode == 0)
> + blob_sizes.lbs_inode = sizeof(struct rcu_head);
> + lsm_set_size(>lbs_inode, _sizes.lbs_inode);
> }
>
> /**
> @@ -311,6 +324,46 @@ int lsm_file_alloc(struct file *file)
> return 0;
> }
>
> +/**
> + * lsm_inode_alloc - allocate a composite inode blob
> + * @inode: the inode that needs a blob
> + *
> + * Allocate the inode blob for all the modules
> + *
> + * Returns 0, or -ENOMEM if memory can't be allocated.
> + */
> +int lsm_inode_alloc(struct inode *inode)
> +{
> + if (!lsm_inode_cache) {
WARN_ON?
> + inode->i_security = NULL;
The other patch didn't set to NULL. Probably should do that in the
other one too.
> + return 0;
> + }
> +
> + inode->i_security = kmem_cache_zalloc(lsm_inode_cache, GFP_NOFS);
> + if (inode->i_security == NULL)
> + return -ENOMEM;
> + return 0;
> +}
> +
> +/**
> + * lsm_early_inode - during initialization allocate a composite inode blob
> + * @inode: the inode that needs a blob
> + *
> + * Allocate the inode blob for all the modules if it's not already there
> + */
> +void lsm_early_inode(struct inode *inode)
> +{
> + int rc;
> +
> + if (inode == NULL)
> + panic("%s: NULL inode.\n", __func__);
> + if (inode->i_security != NULL)
> + return;
> + rc = lsm_inode_alloc(inode);
> + if (rc)
> + panic("%s: Early inode alloc failed.\n", __func__);
> +}
Same thoughts on the use of panic() as the other patch...
> +
> /*
> * Hook list operation macros.
> *
> @@ -557,14 +610,40 @@ EXPORT_SYMBOL(security_sb_parse_opts_str);
>
> int
Re: [PATCH 04/10] LSM: Infrastructure management of the cred security blob
On Tue, Sep 11, 2018 at 9:41 AM, Casey Schaufler wrote:
> Move management of the cred security blob out of the
> security modules and into the security infrastructure.
> Instead of allocating and freeing space the security
> modules tell the infrastructure how much space they
> require.
There's a lot of changes here that I think deserve some longer
discussion in the changelog. Notably, now we run _two_ cycles of init
calls? That seems... odd. Notes below...
> Some SELinux memory management debug code has been removed.
>
> Signed-off-by: Casey Schaufler
> ---
> include/linux/lsm_hooks.h | 14
> kernel/cred.c | 13
> security/Kconfig | 11
> security/apparmor/domain.c| 2 +-
> security/apparmor/include/cred.h | 16 -
> security/apparmor/lsm.c | 28 ++--
> security/apparmor/task.c | 6 +-
> security/security.c | 106 +-
> security/selinux/hooks.c | 63 +-
> security/selinux/include/objsec.h | 4 ++
> security/selinux/selinuxfs.c | 1 +
> security/smack/smack.h| 1 +
> security/smack/smack_lsm.c| 85 +---
> security/tomoyo/common.h | 21 +-
> security/tomoyo/domain.c | 4 +-
> security/tomoyo/securityfs_if.c | 15 +++--
> security/tomoyo/tomoyo.c | 56 +---
> 17 files changed, 303 insertions(+), 143 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 97a020c616ad..0bef312efd45 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2024,6 +2024,13 @@ struct security_hook_list {
> char*lsm;
> } __randomize_layout;
>
> +/*
> + * Security blob size or offset data.
> + */
> +struct lsm_blob_sizes {
> + int lbs_cred;
> +};
> +
> /*
> * Initializing a security_hook_list structure takes
> * up a lot of space in a source file. This macro takes
> @@ -2036,6 +2043,7 @@ struct security_hook_list {
> extern struct security_hook_heads security_hook_heads;
> extern char *lsm_names;
>
> +extern void security_add_blobs(struct lsm_blob_sizes *needed);
> extern void security_add_hooks(struct security_hook_list *hooks, int count,
> char *lsm);
>
> @@ -2082,4 +2090,10 @@ void __init loadpin_add_hooks(void);
> static inline void loadpin_add_hooks(void) { };
> #endif
>
> +extern int lsm_cred_alloc(struct cred *cred, gfp_t gfp);
> +
> +#ifdef CONFIG_SECURITY
> +void lsm_early_cred(struct cred *cred);
> +#endif
> +
> #endif /* ! __LINUX_LSM_HOOKS_H */
> diff --git a/kernel/cred.c b/kernel/cred.c
> index ecf03657e71c..fa2061ee4955 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -704,19 +704,6 @@ bool creds_are_invalid(const struct cred *cred)
> {
> if (cred->magic != CRED_MAGIC)
> return true;
> -#ifdef CONFIG_SECURITY_SELINUX
> - /*
> -* cred->security == NULL if security_cred_alloc_blank() or
> -* security_prepare_creds() returned an error.
> -*/
> - if (selinux_is_enabled() && cred->security) {
> - if ((unsigned long) cred->security < PAGE_SIZE)
> - return true;
> - if ((*(u32 *)cred->security & 0xff00) ==
> - (POISON_FREE << 24 | POISON_FREE << 16 | POISON_FREE <<
> 8))
> - return true;
These aren't unreasonable checks -- can we add them back in later?
(They don't need to be selinux specific, in fact: the LSM could do the
poison...)
> [...]
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 08c88de0ffda..726910bba84b 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -975,7 +975,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
> }
> aa_put_label(cred_label(bprm->cred));
> /* transfer reference, released when cred is freed */
> - cred_label(bprm->cred) = new;
> + set_cred_label(bprm->cred, new);
>
> done:
> aa_put_label(label);
> diff --git a/security/apparmor/include/cred.h
> b/security/apparmor/include/cred.h
> index e287b7d0d4be..a90eae76d7c1 100644
> --- a/security/apparmor/include/cred.h
> +++ b/security/apparmor/include/cred.h
> @@ -23,8 +23,22 @@
> #include "policy_ns.h"
> #include "task.h"
>
> -#define cred_label(X) ((X)->security)
> +static inline struct aa_label *cred_label(const struct cred *cred)
> +{
> + struct aa_label **blob = cred->security;
> +
> + AA_BUG(!blob);
> + return *blob;
> +}
>
> +static inline void set_cred_label(const struct cred *cred,
> + struct aa_label *label)
> +{
> + struct aa_label **blob = cred->security;
> +
> + AA_BUG(!blob);
> + *blob = label;
> +}
This feels like it should be a separate patch? Shouldn't AA not be
Re: [PATCH 03/10] SELinux: Abstract use of cred security blob
On Tue, Sep 11, 2018 at 9:41 AM, Casey Schaufler wrote: > Don't use the cred->security pointer directly. > Provide a helper function that provides the security blob pointer. > > Signed-off-by: Casey Schaufler Like smack, this seems to be largely: s/$identifier->security/selinux_cred($identifier)/ s/current_security()/selinux_cred(current_cred())/ Is that right? The one __task_cred() use seemed to be fully contained under rcu read lock. Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 01/10] procfs: add smack subdir to attrs
On Tue, Sep 11, 2018 at 9:41 AM, Casey Schaufler wrote:
> Back in 2007 I made what turned out to be a rather serious
> mistake in the implementation of the Smack security module.
> The SELinux module used an interface in /proc to manipulate
> the security context on processes. Rather than use a similar
> interface, I used the same interface. The AppArmor team did
> likewise. Now /proc/.../attr/current will tell you the
> security "context" of the process, but it will be different
> depending on the security module you're using.
>
> This patch provides a subdirectory in /proc/.../attr for
> Smack. Smack user space can use the "current" file in
> this subdirectory and never have to worry about getting
> SELinux attributes by mistake. Programs that use the
> old interface will continue to work (or fail, as the case
> may be) as before.
>
> The proposed S.A.R.A security module is dependent on
> the mechanism to create its own attr subdirectory.
>
> The original implementation is by Kees Cook.
>
> Signed-off-by: Casey Schaufler
> ---
> Documentation/admin-guide/LSM/index.rst | 13 +++--
> fs/proc/base.c | 64 +
> fs/proc/internal.h | 1 +
> include/linux/security.h| 15 --
> security/security.c | 24 --
> 5 files changed, 96 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/admin-guide/LSM/index.rst
> b/Documentation/admin-guide/LSM/index.rst
> index c980dfe9abf1..9842e21afd4a 100644
> --- a/Documentation/admin-guide/LSM/index.rst
> +++ b/Documentation/admin-guide/LSM/index.rst
> @@ -17,9 +17,8 @@ MAC extensions, other extensions can be built using the LSM
> to provide
> specific changes to system operation when these tweaks are not available
> in the core functionality of Linux itself.
>
> -Without a specific LSM built into the kernel, the default LSM will be the
> -Linux capabilities system. Most LSMs choose to extend the capabilities
> -system, building their checks on top of the defined capability hooks.
> +The Linux capabilities modules will always be included. This may be
> +followed by any number of "minor" modules and at most one "major" module.
> For more details on capabilities, see ``capabilities(7)`` in the Linux
> man-pages project.
>
> @@ -30,6 +29,14 @@ order in which checks are made. The capability module will
> always
> be first, followed by any "minor" modules (e.g. Yama) and then
> the one "major" module (e.g. SELinux) if there is one configured.
>
> +Process attributes associated with "major" security modules should
> +be accessed and maintained using the special files in ``/proc/.../attr``.
> +A security module may maintain a module specific subdirectory there,
> +named after the module. ``/proc/.../attr/smack`` is provided by the Smack
> +security module and contains all its special files. The files directly
> +in ``/proc/.../attr`` remain as legacy interfaces for modules that provide
> +subdirectories.
> +
> .. toctree::
> :maxdepth: 1
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index ccf86f16d9f0..bd2dd85310fe 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -140,9 +140,13 @@ struct pid_entry {
> #define REG(NAME, MODE, fops) \
> NOD(NAME, (S_IFREG|(MODE)), NULL, , {})
> #define ONE(NAME, MODE, show) \
> - NOD(NAME, (S_IFREG|(MODE)), \
> + NOD(NAME, (S_IFREG|(MODE)), \
> NULL, _single_file_operations, \
> { .proc_show = show } )
> +#define ATTR(LSM, NAME, MODE) \
> + NOD(NAME, (S_IFREG|(MODE)), \
> + NULL, _pid_attr_operations,\
> + { .lsm = LSM })
>
> /*
> * Count the number of hardlinks for the pid_entry table, excluding the .
> @@ -2503,7 +2507,7 @@ static ssize_t proc_pid_attr_read(struct file * file,
> char __user * buf,
> if (!task)
> return -ESRCH;
>
> - length = security_getprocattr(task,
> + length = security_getprocattr(task, PROC_I(inode)->op.lsm,
> (char*)file->f_path.dentry->d_name.name,
> );
> put_task_struct(task);
> @@ -2552,7 +2556,9 @@ static ssize_t proc_pid_attr_write(struct file * file,
> const char __user * buf,
> if (rv < 0)
> goto out_free;
>
> - rv = security_setprocattr(file->f_path.dentry->d_name.name, page,
> count);
> + rv = security_setprocattr(PROC_I(inode)->op.lsm,
> + file->f_path.dentry->d_name.name, page,
> + count);
> mutex_unlock(>signal->cred_guard_mutex);
> out_free:
> kfree(page);
> @@ -2566,13 +2572,53 @@ static const struct file_operations
> proc_pid_attr_operations = {
> .llseek =
Re: [PATCH 02/10] Smack: Abstract use of cred security blob
On Tue, Sep 11, 2018 at 9:41 AM, Casey Schaufler wrote:
> Don't use the cred->security pointer directly.
> Provide a helper function that provides the security blob pointer.
>
> Signed-off-by: Casey Schaufler
> ---
> security/smack/smack.h| 14 +++--
> security/smack/smack_access.c | 4 +--
> security/smack/smack_lsm.c| 57 +--
> security/smack/smackfs.c | 18 +--
> 4 files changed, 50 insertions(+), 43 deletions(-)
>
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index f7db791fb566..0b55d6a55b26 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -356,6 +356,11 @@ extern struct list_head smack_onlycap_list;
> #define SMACK_HASH_SLOTS 16
> extern struct hlist_head smack_known_hash[SMACK_HASH_SLOTS];
>
> +static inline struct task_smack *smack_cred(const struct cred *cred)
> +{
> + return cred->security;
> +}
> +
> /*
> * Is the directory transmuting?
> */
> @@ -382,13 +387,16 @@ static inline struct smack_known *smk_of_task(const
> struct task_smack *tsp)
> return tsp->smk_task;
> }
>
> -static inline struct smack_known *smk_of_task_struct(const struct
> task_struct *t)
> +static inline struct smack_known *smk_of_task_struct(
> + const struct task_struct *t)
> {
> struct smack_known *skp;
> + const struct cred *cred;
>
> rcu_read_lock();
> - skp = smk_of_task(__task_cred(t)->security);
> + cred = __task_cred(t);
> rcu_read_unlock();
> + skp = smk_of_task(smack_cred(cred));
Hm, why is this safe? (i.e. what is pinning the cred?) I would expect
get_cred()/put_cred() since this is not for "current"? And then what
controls the skp lifetime?
Everything else looks to be mechanical replacement, so that's fine.
Did you use some tooling to do the mechanical replacement or was it
done by hand?
-Kees
--
Kees Cook
Pixel Security
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v2 00/10] LSM: Module stacking in support of S.A.R.A and Landlock
Adding the SARA and LandLock authors for review & comment. Salvatore & Mickaël: does this patchset meet your needs for merging to mainline? On Tue, 11 Sep 2018, Casey Schaufler wrote: > LSM: Module stacking in support of S.A.R.A and Landlock > > v2: Reduce the patchset to what is required to support > the proposed S.A.R.A. and LandLock security modules > > The S.A.R.A. security module is intended to be used > in conjunction with other security modules. It requires > state to be maintained for the credential, which > in turn requires a mechanism for sharing the credential > security blob. The module also requires mechanism for > user space manipulation of the credential information, > hence an additional subdirectory in /proc/.../attr. > > The LandLock security module provides user configurable > policy in the secmark mechanism. It requires data in > the credential, file and inode security blobs. For this > to be used along side the existing "major" security > modules mechanism for sharing these blobs is provided. > > A side effect of providing sharing of the crendential > security blob is that the TOMOYO module can be used at > the same time as the other "major" modules. > > The mechanism for configuring which security modules are > enabled has to change when stacking in enabled. Any > module that uses just the security blobs that are shared > can be selected. Additionally, one other "major" module > can be selected. > > The security module stacking issues around networking and > IPC are not addressed here as they are beyond what is > required for TOMOYO, S.A.R.A and LandLock. > > git://github.com/cschaufler/lsm-stacking.git#stacking-4.19-rc2-saralock > > Signed-off-by: Casey Schaufler > --- > Documentation/admin-guide/LSM/index.rst | 23 ++- > fs/proc/base.c | 64 ++- > fs/proc/internal.h | 1 + > include/linux/lsm_hooks.h | 20 ++- > include/linux/security.h| 15 +- > kernel/cred.c | 13 -- > security/Kconfig| 92 ++ > security/apparmor/domain.c | 2 +- > security/apparmor/include/cred.h| 24 ++- > security/apparmor/include/file.h| 9 +- > security/apparmor/include/lib.h | 4 + > security/apparmor/lsm.c | 53 -- > security/apparmor/task.c| 6 +- > security/security.c | 293 > ++-- > security/selinux/hooks.c| 215 --- > security/selinux/include/objsec.h | 37 +++- > security/selinux/selinuxfs.c| 5 +- > security/selinux/xfrm.c | 4 +- > security/smack/smack.h | 42 - > security/smack/smack_access.c | 4 +- > security/smack/smack_lsm.c | 283 +++--- > security/smack/smackfs.c| 18 +- > security/tomoyo/common.h| 31 +++- > security/tomoyo/domain.c| 4 +- > security/tomoyo/securityfs_if.c | 15 +- > security/tomoyo/tomoyo.c| 57 +-- > 26 files changed, 899 insertions(+), 435 deletions(-) > -- James Morris ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
On 09/13/2018 08:26 AM, Tetsuo Handa wrote: > On 2018/09/13 12:02, Paul Moore wrote: >> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa >> wrote: >>> syzbot is hitting warning at str_read() [1] because len parameter can >>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for >>> this case. >>> >>> [1] >>> https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 >>> >>> Signed-off-by: Tetsuo Handa >>> Reported-by: syzbot >>> --- >>> security/selinux/ss/policydb.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c >>> index e9394e7..f4eadd3 100644 >>> --- a/security/selinux/ss/policydb.c >>> +++ b/security/selinux/ss/policydb.c >>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void >>> *fp, u32 len) >>> if ((len == 0) || (len == (u32)-1)) >>> return -EINVAL; >>> >>> - str = kmalloc(len + 1, flags); >>> + str = kmalloc(len + 1, flags | __GFP_NOWARN); >>> if (!str) >>> return -ENOMEM; >> Thanks for the patch. >> >> My eyes are starting to glaze over a bit chasing down all of the >> different kmalloc() code paths trying to ensure that this always does >> the right thing based on size of the allocation and the different slab >> allocators ... are we sure that this will always return NULL when (len >> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator >> configurations? >> > Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return > ZERO_SIZE_PTR) due to (len == (u32)-1) check above. > > The only concern would be whether you want allocation failure messages. > I assumed you don't need it because we are returning -ENOMEM to the caller. > Would it not be better with char *str; if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE)) return -EINVAL; str = kmalloc(len + 1, flags); if (!str) return -ENOMEM; ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
