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

[Paul Moore]

On Mon, Oct 31, 2022 at 3:22 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, Oct 31, 2022 at 4:07 AM Paul Moore <paul@xxxxxxxxxxxxxx> 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.

-- 
paul-moore.com