Message ID | CAHC9VhSSafrkW4VbTVoAUJmjFQdCwPTGDqTP8yBnLBqc7rW7iQ@mail.gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp2246890wru; Mon, 31 Oct 2022 04:12:16 -0700 (PDT) X-Google-Smtp-Source: AMsMyM4F8hLz3ZV36yjUT0i9HNu996nX8ttOQJx/8+u0YzwKfByeZw2m9v2ForFQsd9KK2pUQ7b2 X-Received: by 2002:a17:906:58c7:b0:722:f4bf:cb75 with SMTP id e7-20020a17090658c700b00722f4bfcb75mr12168490ejs.450.1667214736628; Mon, 31 Oct 2022 04:12:16 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1667214736; cv=none; d=google.com; s=arc-20160816; b=Vsq7VyX7FZj5IqMNB4TuxARR+3394GbVNwq2m4sH9vbrUj+YxV8SozlYmEm8D2cNK/ wim/VnGNECkL8lLIqP4BWSRYlA4KIPjiNtRlkNModPtfT0of/4zH+ej6SbM55xV2enkE H4fXmPmNii4VdDyvrkHwUqPymRli8yy7qjGmG0+haGyOpp/rkG4vFETbArn+dY4xTkUx RDk33FZhNes9JaW4c4ZdoNTp5s+e9dyCtdA+mC/fW/M1zJcy0pNKxZXzEt5AFqwyHeCU islPFBo9/21eS8YO9J4M2u+Rl2UbxtUKonHKHXdtkR3QcZNcA8GU05jDjrD4JjPAnCiK 9gAw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:mime-version :dkim-signature; bh=sMlKM22/vBjYG6XqmR4GF/IFfdYTzBgjEp3g+APsfwk=; b=bc5GceBe+E7xHRXEGSHteorzOVtBCP4SCMXjb44A2FiXc1wmSDXU6kJ9fU0o8EJWAf 86xS5l342zKk0GoAAdOecfj9hv8sBj9xDm54OmxPcg9ed2XKi9RRjRxZGbbkkcZmEOuI c0TtDJSemUoTF5XoEEudPY7TaDwi6O5DSW8I0JfZqn2AqPb96r9SQxM1bEFBOVQTwEa8 VRM1K8EtfUTgKGa1fZMTTJab9+9jvztgnSgs1MW53h2MK85S7D8e6F0BC9MCmOCu1bgh PTCW0YJEpvGzgob3SPjOSV0v0w2ioaJItn6K5VkGEBNgYn+K7V36ZMScHVHewEedpF/4 IwXA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@paul-moore-com.20210112.gappssmtp.com header.s=20210112 header.b=mWkrrb+U; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id y4-20020a056402440400b0045bd677b3d5si9177843eda.342.2022.10.31.04.11.52; Mon, 31 Oct 2022 04:12:16 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@paul-moore-com.20210112.gappssmtp.com header.s=20210112 header.b=mWkrrb+U; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229974AbiJaLH3 (ORCPT <rfc822;kartikey406@gmail.com> + 99 others); Mon, 31 Oct 2022 07:07:29 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52146 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229743AbiJaLH2 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 31 Oct 2022 07:07:28 -0400 Received: from mail-oa1-x34.google.com (mail-oa1-x34.google.com [IPv6:2001:4860:4864:20::34]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3286862CF for <linux-kernel@vger.kernel.org>; Mon, 31 Oct 2022 04:07:27 -0700 (PDT) Received: by mail-oa1-x34.google.com with SMTP id 586e51a60fabf-13c569e5ff5so12666546fac.6 for <linux-kernel@vger.kernel.org>; Mon, 31 Oct 2022 04:07:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=paul-moore-com.20210112.gappssmtp.com; s=20210112; h=cc:to:subject:message-id:date:from:mime-version:from:to:cc:subject :date:message-id:reply-to; bh=sMlKM22/vBjYG6XqmR4GF/IFfdYTzBgjEp3g+APsfwk=; b=mWkrrb+U58/aLUgoZbnCvf+7PUeEpgFmzXTIE3ZeDRp4An6IcGTQmNRWTB83Y30Eri rTXYdP4rlXt57vBoGvr/5R541P75FYw6l8H3P+fZ9kqvH3u3LiXysiWr8aN9+3zU0N5X DJ6qF0MpXic2BxobXvP3eMdkjftnOtOp+DXhXOnNR2gRqq/vn72snijzHCv0ym7/pE0o AecgDw4P/dezc1b4sWKjDKY44W25sXZpRuGUA26wvwBDnZlNGIYYS1x+WQ4tI+1jDNZz OM8svpGeIXkCZZxmEJUmDRU5ziUBShV8UL+8FABoIoFLEdGaA1siE/lj+VHICkNjJP6u BVHw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:mime-version:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=sMlKM22/vBjYG6XqmR4GF/IFfdYTzBgjEp3g+APsfwk=; b=tZZD0QMJcVBPlCzxsLwjmzqvs3fldESovDvdIgZyWmCVZ3NqGHdX7Xz0X65hw9Vfpn BiSsOaTEHJoEZi8RPc22LfczTMe3tkMjCaT2oZuazYuvtzHaHUN9le4Ll1nheAb8ZOYl H8thzXiLG8LU2v4fLahcNA1zTOmNX8M5MtzZfZZUwgjqCxtU02HrrWdjpstUOO281RG0 MZdvkUQeOuF0fxXslnbpnjHfgwtuzKvVgXcPLjQOnPPQGyP935fcCm1GTZXUkUlK6K3m 04HXzH0Tm5lFOFIR7Fdw4dr05G808qzvddE8pzX9JGal5jNKUOA9stcE3gY+ErReLBNw EbWQ== X-Gm-Message-State: ACrzQf3PEc66eMB6guLxqKkSk1Oym7iNf5yXtNu/8RjbwXESnLHTvYKd WlMpkBxu2nT7qouh/XokaUVdOEZJPtKHFVHK35dzhdGkRl73zrA= X-Received: by 2002:a05:6870:41cb:b0:131:9656:cc30 with SMTP id z11-20020a05687041cb00b001319656cc30mr17158926oac.51.1667214446436; Mon, 31 Oct 2022 04:07:26 -0700 (PDT) MIME-Version: 1.0 From: Paul Moore <paul@paul-moore.com> Date: Mon, 31 Oct 2022 07:07:15 -0400 Message-ID: <CAHC9VhSSafrkW4VbTVoAUJmjFQdCwPTGDqTP8yBnLBqc7rW7iQ@mail.gmail.com> Subject: [GIT PULL] LSM fixes for v6.1 (#1) To: Linus Torvalds <torvalds@linux-foundation.org> Cc: linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1748201359735985562?= X-GMAIL-MSGID: =?utf-8?q?1748201359735985562?= |
Series |
[GIT,PULL] LSM fixes for v6.1 (#1)
|
|
Pull-request
git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git tags/lsm-pr-20221031Message
Paul Moore
Oct. 31, 2022, 11:07 a.m. UTC
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(-)
Comments
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.