[PATCH] selinux: check for failed kmalloc in security_sid_to_context

[PATCH] selinux: check for failed kmalloc in security_sid_to_context

[Serge E. Hallyn]

Check for NULL kmalloc return value before writing to it.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

 security/selinux/ss/services.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

3d9cf05c7fa2578f87648dd0862e70cf7959ad7a
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 6149248..20b1065 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -593,6 +593,10 @@ int security_sid_to_context(u32 sid, cha
 
 			*scontext_len = strlen(initial_sid_to_string[sid]) + 1;
 			scontextp = kmalloc(*scontext_len,GFP_ATOMIC);
+			if (!scontextp) {
+				rc = -ENOMEM;
+				goto out;
+			}
 			strcpy(scontextp, initial_sid_to_string[sid]);
 			*scontext = scontextp;
 			goto out;
-- 
1.3.0

Re: [PATCH] selinux: check for failed kmalloc in security_sid_to_context

[James Morris]

On Wed, 26 Apr 2006, Serge E. Hallyn wrote:

> Check for NULL kmalloc return value before writing to it.
> 
> Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

Acked-by: James Morris <jmorris@namei.org>


> ---
> 
>  security/selinux/ss/services.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> 3d9cf05c7fa2578f87648dd0862e70cf7959ad7a
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 6149248..20b1065 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -593,6 +593,10 @@ int security_sid_to_context(u32 sid, cha
>  
>  			*scontext_len = strlen(initial_sid_to_string[sid]) + 1;
>  			scontextp = kmalloc(*scontext_len,GFP_ATOMIC);
> +			if (!scontextp) {
> +				rc = -ENOMEM;
> +				goto out;
> +			}
>  			strcpy(scontextp, initial_sid_to_string[sid]);
>  			*scontext = scontextp;
>  			goto out;
> 

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] selinux: check for failed kmalloc in security_sid_to_context

[Andrew Morton]

James Morris <jmorris@namei.org> wrote:
>
> On Wed, 26 Apr 2006, Serge E. Hallyn wrote:
> 
> > Check for NULL kmalloc return value before writing to it.
> > 
> > Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
> 
> Acked-by: James Morris <jmorris@namei.org>
> 
> 
> > ---
> > 
> >  security/selinux/ss/services.c |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > 3d9cf05c7fa2578f87648dd0862e70cf7959ad7a
> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index 6149248..20b1065 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -593,6 +593,10 @@ int security_sid_to_context(u32 sid, cha
> >  
> >  			*scontext_len = strlen(initial_sid_to_string[sid]) + 1;
> >  			scontextp = kmalloc(*scontext_len,GFP_ATOMIC);
> > +			if (!scontextp) {
> > +				rc = -ENOMEM;
> > +				goto out;
> > +			}
> >  			strcpy(scontextp, initial_sid_to_string[sid]);
> >  			*scontext = scontextp;
> >  			goto out;
> > 
> 

Given that GFP_ATOMIC can fail and it'll cause an oops I'll queue this for
2.6.17 and shall send it in the direction of the -stable guys too, thanks.

What will happen when one of the GFP_ATOMIC allocations in there fails? 
Will the computer become insecure?

Re: [PATCH] selinux: check for failed kmalloc in security_sid_to_context

[Stephen Smalley]

On Mon, 2006-05-08 at 10:46 -0700, Andrew Morton wrote:
> James Morris <jmorris@namei.org> wrote:
> >
> > On Wed, 26 Apr 2006, Serge E. Hallyn wrote:
> > 
> > > Check for NULL kmalloc return value before writing to it.
> > > 
> > > Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
> > 
> > Acked-by: James Morris <jmorris@namei.org>
> > 
> > 
> > > ---
> > > 
> > >  security/selinux/ss/services.c |    4 ++++
> > >  1 files changed, 4 insertions(+), 0 deletions(-)
> > > 
> > > 3d9cf05c7fa2578f87648dd0862e70cf7959ad7a
> > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > > index 6149248..20b1065 100644
> > > --- a/security/selinux/ss/services.c
> > > +++ b/security/selinux/ss/services.c
> > > @@ -593,6 +593,10 @@ int security_sid_to_context(u32 sid, cha
> > >  
> > >  			*scontext_len = strlen(initial_sid_to_string[sid]) + 1;
> > >  			scontextp = kmalloc(*scontext_len,GFP_ATOMIC);
> > > +			if (!scontextp) {
> > > +				rc = -ENOMEM;
> > > +				goto out;
> > > +			}
> > >  			strcpy(scontextp, initial_sid_to_string[sid]);
> > >  			*scontext = scontextp;
> > >  			goto out;
> > > 
> > 
> 
> Given that GFP_ATOMIC can fail and it'll cause an oops I'll queue this for
> 2.6.17 and shall send it in the direction of the -stable guys too, thanks.

Note however that this can only occur prior to initial policy load
by /sbin/init; after that, we don't follow that branch.

> What will happen when one of the GFP_ATOMIC allocations in there fails? 
> Will the computer become insecure?

No, it doesn't affect the access control enforcement.  The caller just
can't get the context string in that case, so in the case of the AVC or
audit, it falls back to only logging the SID for later analysis.  The
audit people are interested in having a way to dump the kernel's SID
table in that case, so we may end up exporting that via selinuxfs.  In
some other cases where we are returning the context to userspace, the
program would get the ENOMEM error ultimately and have to retry when
memory was available.

-- 
Stephen Smalley
National Security Agency