| Message ID | 20200414142351.162526-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | selinux: free str on error in str_read() | expand |
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 >
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.
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.
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.
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; }
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(-)