[GIT,PULL] LSM fixes for v6.1 (#1)

  Hi Linus,

A single patch to the capabilities code to fix a potential memory leak
in the xattr allocation error handling.  Please apply for v6.1-rcX.

Thanks,
-Paul

--
The following changes since commit 9abf2313adc1ca1b6180c508c25f22f9395cc780:

 Linux 6.1-rc1 (2022-10-16 15:36:24 -0700)

are available in the Git repository at:

 git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git
   tags/lsm-pr-20221031

for you to fetch changes up to 8cf0a1bc12870d148ae830a4ba88cfdf0e879cee:

 capabilities: fix potential memleak on error path from vfs_getxattr_alloc()
   (2022-10-28 06:44:33 -0400)

----------------------------------------------------------------
lsm/stable-6.1 PR 20221031

----------------------------------------------------------------
Gaosheng Cui (1):
     capabilities: fix potential memleak on error path from
                   vfs_getxattr_alloc()

security/commoncap.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
  

On Mon, Oct 31, 2022 at 4:07 AM Paul Moore <paul@paul-moore.com> wrote:
>
> A single patch to the capabilities code to fix a potential memory leak
> in the xattr allocation error handling.  Please apply for v6.1-rcX.

Pulled.

However, I react to the strange test condition. Sure, it's
pre-existing, but does it really make sense?

It does

+       if (ret < 0 || !tmpbuf) {
+               size = ret;
+               goto out_free;
+       }

and how the heck can 'tmpbuf' be NULL if vfs_getxattr_alloc() succeeded?

I think that's not only impossible in the first place, but if it *was*
possible, then that

                size = ret;
               goto out_free;

would be wrong, because this function would return success even if it
wasn't successful.

That whole "cast to int, and then cast back to size_t" also smells of
some serious confusion in the return value handling. It looks to me
like vfs_getxattr_alloc() fundamentally returns an 'int', not a
'ssize_t', just by looking at the ->get function. But it just all
looks weird.

So this code has all kinds of oddities.

               Linus
  

The pull request you sent on Mon, 31 Oct 2022 07:07:15 -0400:

> git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git tags/lsm-pr-20221031

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/78a089d033bf71d68d978ac4cc73070f3e71c736

Thank you!
  

On Mon, Oct 31, 2022 at 3:22 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, Oct 31, 2022 at 4:07 AM Paul Moore <paul@paul-moore.com> wrote:
> >
> > A single patch to the capabilities code to fix a potential memory leak
> > in the xattr allocation error handling.  Please apply for v6.1-rcX.
>
> Pulled.

Sorry for the delay in responding, you saw this in my other response,
but limited network access, yadda yadda ...

> However, I react to the strange test condition. Sure, it's
> pre-existing, but does it really make sense?

I wasn't responsible for this code when the conditional was written,
and I've got enough mail in my backlog at the moment to not want to
sift through the git log trying to make sense of it, but the current
conditional does seem a bit "extra" when one considers
vfs_getxattr_alloc().  The only gotcha that I can see is that
vfs_getxattr_alloc() callers need to ensure that they always kfree()
the xattr_value buffer on error as vfs_getxattr_alloc() may leave
memory allocated on failure.  There was discussion of that when this
leak fix patch was posted.

I'll put together a cleanup patch to resolve the conditional oddity
and send it up during the next merge window.

> That whole "cast to int, and then cast back to size_t" also smells of
> some serious confusion in the return value handling. It looks to me
> like vfs_getxattr_alloc() fundamentally returns an 'int', not a
> 'ssize_t', just by looking at the ->get function. But it just all
> looks weird.

Yes, it's a bit of a mess.  I suspect the problem originated in that
vfs_getxattr_alloc() returns either a negative number on failure or
the size of the allocation on success, and with allocation sizes
typically using a {s}size_t type I'm guessing the original authors
chose ssize_t, which seems reasonable until one looks at the
xattr_handler's ->get() function and realizes that it returns an int.

I think the right thing to do here is to update vfs_getxattr_alloc()
to use an int return type and update all of the callers accordingly
(currently they all live under security/).  I'll put together a patch
to clean this up and send it via the next merge window assuming there
are no objections to the patch.
  

On Mon, Oct 31, 2022 at 12:22:29PM -0700, Linus Torvalds wrote:
> On Mon, Oct 31, 2022 at 4:07 AM Paul Moore <paul@paul-moore.com> wrote:
> >
> > A single patch to the capabilities code to fix a potential memory leak
> > in the xattr allocation error handling.  Please apply for v6.1-rcX.
> 
> Pulled.
> 
> However, I react to the strange test condition. Sure, it's
> pre-existing, but does it really make sense?
> 
> It does
> 
> +       if (ret < 0 || !tmpbuf) {
> +               size = ret;
> +               goto out_free;
> +       }
> 
> and how the heck can 'tmpbuf' be NULL if vfs_getxattr_alloc() succeeded?

I had to go through the history a bit - the !tmpbuf check was added

https://www.spinics.net/lists/stable/msg463010.html

because of a gcc warning.  Perhaps there's a better way to tell gcc
that it can't remain NULL if ret was < 0 ?

> I think that's not only impossible in the first place, but if it *was*
> possible, then that
> 
>                 size = ret;
>                goto out_free;
> 
> would be wrong, because this function would return success even if it
> wasn't successful.
> 
> That whole "cast to int, and then cast back to size_t" also smells of
> some serious confusion in the return value handling. It looks to me
> like vfs_getxattr_alloc() fundamentally returns an 'int', not a
> 'ssize_t', just by looking at the ->get function. But it just all
> looks weird.
> 
> So this code has all kinds of oddities.
> 
>                Linus
  

On Wed, Nov 9, 2022 at 9:38 AM Serge E. Hallyn <serge@hallyn.com> wrote:
> On Mon, Oct 31, 2022 at 12:22:29PM -0700, Linus Torvalds wrote:
> > On Mon, Oct 31, 2022 at 4:07 AM Paul Moore <paul@paul-moore.com> wrote:
> > >
> > > A single patch to the capabilities code to fix a potential memory leak
> > > in the xattr allocation error handling.  Please apply for v6.1-rcX.
> >
> > Pulled.
> >
> > However, I react to the strange test condition. Sure, it's
> > pre-existing, but does it really make sense?
> >
> > It does
> >
> > +       if (ret < 0 || !tmpbuf) {
> > +               size = ret;
> > +               goto out_free;
> > +       }
> >
> > and how the heck can 'tmpbuf' be NULL if vfs_getxattr_alloc() succeeded?
>
> I had to go through the history a bit - the !tmpbuf check was added
>
> https://www.spinics.net/lists/stable/msg463010.html
>
> because of a gcc warning.  Perhaps there's a better way to tell gcc
> that it can't remain NULL if ret was < 0 ?

Ooof, that's ugly, but thanks for digging it up.  As it turns out I
happen to be working on a patch for vfs_getxattr_alloc() to fix the
return value type right now, but it looks like I'll leave that gcc
hack in place ... although I might leave a comment about it so the
next person doesn't have to wonder.

> > I think that's not only impossible in the first place, but if it *was*
> > possible, then that
> >
> >                 size = ret;
> >                goto out_free;
> >
> > would be wrong, because this function would return success even if it
> > wasn't successful.
> >
> > That whole "cast to int, and then cast back to size_t" also smells of
> > some serious confusion in the return value handling. It looks to me
> > like vfs_getxattr_alloc() fundamentally returns an 'int', not a
> > 'ssize_t', just by looking at the ->get function. But it just all
> > looks weird.
> >
> > So this code has all kinds of oddities.
> >
> >                Linus
  

On Wed, Nov 9, 2022 at 3:13 PM Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Nov 9, 2022 at 9:38 AM Serge E. Hallyn <serge@hallyn.com> wrote:
> > On Mon, Oct 31, 2022 at 12:22:29PM -0700, Linus Torvalds wrote:
> > > On Mon, Oct 31, 2022 at 4:07 AM Paul Moore <paul@paul-moore.com> wrote:
> > > >
> > > > A single patch to the capabilities code to fix a potential memory leak
> > > > in the xattr allocation error handling.  Please apply for v6.1-rcX.
> > >
> > > Pulled.
> > >
> > > However, I react to the strange test condition. Sure, it's
> > > pre-existing, but does it really make sense?
> > >
> > > It does
> > >
> > > +       if (ret < 0 || !tmpbuf) {
> > > +               size = ret;
> > > +               goto out_free;
> > > +       }
> > >
> > > and how the heck can 'tmpbuf' be NULL if vfs_getxattr_alloc() succeeded?
> >
> > I had to go through the history a bit - the !tmpbuf check was added
> >
> > https://www.spinics.net/lists/stable/msg463010.html
> >
> > because of a gcc warning.  Perhaps there's a better way to tell gcc
> > that it can't remain NULL if ret was < 0 ?
>
> Ooof, that's ugly, but thanks for digging it up.  As it turns out I
> happen to be working on a patch for vfs_getxattr_alloc() to fix the
> return value type right now, but it looks like I'll leave that gcc
> hack in place ... although I might leave a comment about it so the
> next person doesn't have to wonder.

Actually, it looks like there are other similar conditions, e.g.
evm_is_immutable(), without such a check and my compiler (gcc v12.2.0)
seems okay with it; presumably they fixed the compiler bug?

I guess I'll leave the hack in place for commoncap.c but not propagate
it elsewhere.