| Message ID | CAHC9VhSSafrkW4VbTVoAUJmjFQdCwPTGDqTP8yBnLBqc7rW7iQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [GIT,PULL] LSM fixes for v6.1 (#1) | expand |
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.
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(-)