[PATCH] selinux: free str on error in str_read()

[PATCH] selinux: free str on error in str_read()

[Ondrej Mosnacek]

In [see "Fixes:"] I missed the fact that str_read() may give back an
allocated pointer even if it returns an error, causing a potential
memory leak in filename_trans_read_one(). Fix this by making the
function free the allocated string whenever it returns a non-zero value,
which also makes its behavior more obvious and prevents repeating the
same mistake in the future.

Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
Addresses-Coverity-ID: 1461665 ("Resource leaks")
Fixes: c3a276111ea2 ("selinux: optimize storage of filename transitions")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/ss/policydb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 70ecdc78efbd..c21b922e5ebe 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1035,14 +1035,14 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
 	if (!str)
 		return -ENOMEM;
 
-	/* it's expected the caller should free the str */
-	*strp = str;
-
 	rc = next_entry(str, fp, len);
-	if (rc)
+	if (rc) {
+		kfree(str);
 		return rc;
+	}
 
 	str[len] = '\0';
+	*strp = str;
 	return 0;
 }
 
-- 
2.25.2

Re: [PATCH] selinux: free str on error in str_read()

[Kees Cook]

On Tue, Apr 14, 2020 at 04:23:51PM +0200, Ondrej Mosnacek wrote:
> In [see "Fixes:"] I missed the fact that str_read() may give back an
> allocated pointer even if it returns an error, causing a potential
> memory leak in filename_trans_read_one(). Fix this by making the
> function free the allocated string whenever it returns a non-zero value,
> which also makes its behavior more obvious and prevents repeating the
> same mistake in the future.
> 
> Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
> Addresses-Coverity-ID: 1461665 ("Resource leaks")
> Fixes: c3a276111ea2 ("selinux: optimize storage of filename transitions")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  security/selinux/ss/policydb.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 70ecdc78efbd..c21b922e5ebe 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1035,14 +1035,14 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
>  	if (!str)
>  		return -ENOMEM;
>  
> -	/* it's expected the caller should free the str */
> -	*strp = str;
> -
>  	rc = next_entry(str, fp, len);
> -	if (rc)
> +	if (rc) {
> +		kfree(str);
>  		return rc;
> +	}
>  
>  	str[len] = '\0';
> +	*strp = str;
>  	return 0;
>  }
>  
> -- 
> 2.25.2
> 

-- 
Kees Cook

Re: [PATCH] selinux: free str on error in str_read()

[Paul Moore]

On Tue, Apr 14, 2020 at 10:25 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> In [see "Fixes:"] I missed the fact that str_read() may give back an
> allocated pointer even if it returns an error, causing a potential
> memory leak in filename_trans_read_one(). Fix this by making the
> function free the allocated string whenever it returns a non-zero value,
> which also makes its behavior more obvious and prevents repeating the
> same mistake in the future.
>
> Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
> Addresses-Coverity-ID: 1461665 ("Resource leaks")
> Fixes: c3a276111ea2 ("selinux: optimize storage of filename transitions")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/ss/policydb.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

I just merged this into selinux/stable-5.7 and assuming all goes well
in testing I'll send this up to Linus later this week.  Thanks Ondrej.

I also want to add my thanks to the "coverity bot", thanks Kees.  Are
you only running this only on Linus tree?  If it's open to other trees
it might be nice to get the selinux/next branch into the automated
testing.

-- 
paul moore
www.paul-moore.com

Re: [PATCH] selinux: free str on error in str_read()

[Kees Cook]

On Wed, Apr 15, 2020 at 06:04:53PM -0400, Paul Moore wrote:
> On Tue, Apr 14, 2020 at 10:25 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > In [see "Fixes:"] I missed the fact that str_read() may give back an
> > allocated pointer even if it returns an error, causing a potential
> > memory leak in filename_trans_read_one(). Fix this by making the
> > function free the allocated string whenever it returns a non-zero value,
> > which also makes its behavior more obvious and prevents repeating the
> > same mistake in the future.
> >
> > Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
> > Addresses-Coverity-ID: 1461665 ("Resource leaks")
> > Fixes: c3a276111ea2 ("selinux: optimize storage of filename transitions")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  security/selinux/ss/policydb.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> I just merged this into selinux/stable-5.7 and assuming all goes well
> in testing I'll send this up to Linus later this week.  Thanks Ondrej.
> 
> I also want to add my thanks to the "coverity bot", thanks Kees.  Are
> you only running this only on Linus tree?  If it's open to other trees
> it might be nice to get the selinux/next branch into the automated
> testing.

It's being run on linux-next. The free coverity scanner barely has the
resources is keep up with one tree, so I just feed it -next. They were
kind enough to let us upload daily now, so I've been trying to feed the
emailed reports back. It's all just the tip of the iceberg, of course.

-- 
Kees Cook

Re: [PATCH] selinux: free str on error in str_read()

[Paul Moore]

On Fri, Apr 17, 2020 at 5:47 PM Kees Cook <keescook@chromium.org> wrote:
> On Wed, Apr 15, 2020 at 06:04:53PM -0400, Paul Moore wrote:
> > On Tue, Apr 14, 2020 at 10:25 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > In [see "Fixes:"] I missed the fact that str_read() may give back an
> > > allocated pointer even if it returns an error, causing a potential
> > > memory leak in filename_trans_read_one(). Fix this by making the
> > > function free the allocated string whenever it returns a non-zero value,
> > > which also makes its behavior more obvious and prevents repeating the
> > > same mistake in the future.
> > >
> > > Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
> > > Addresses-Coverity-ID: 1461665 ("Resource leaks")
> > > Fixes: c3a276111ea2 ("selinux: optimize storage of filename transitions")
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > ---
> > >  security/selinux/ss/policydb.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > I just merged this into selinux/stable-5.7 and assuming all goes well
> > in testing I'll send this up to Linus later this week.  Thanks Ondrej.
> >
> > I also want to add my thanks to the "coverity bot", thanks Kees.  Are
> > you only running this only on Linus tree?  If it's open to other trees
> > it might be nice to get the selinux/next branch into the automated
> > testing.
>
> It's being run on linux-next. The free coverity scanner barely has the
> resources is keep up with one tree, so I just feed it -next. They were
> kind enough to let us upload daily now, so I've been trying to feed the
> emailed reports back. It's all just the tip of the iceberg, of course.

Ah, okay, thanks.  I had wondered about doing regular coverity runs
for the SELinux/audit kernel code but was scared off by the limits; it
looks like that wasn't an unwarranted fear.

Regardless, thanks for setting this up and running it on linux-next.

-- 
paul moore
www.paul-moore.com