Message ID | 20221104122023.1750333-1-roberto.sassu@huaweicloud.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 l7csp376486wru; Fri, 4 Nov 2022 05:44:05 -0700 (PDT) X-Google-Smtp-Source: AMsMyM4abSTRHNtazLAeoIzW2U630IGyApDCy9nE9yYMIfk0mSetI3fV5J3EW5AjXywq8T23AoA9 X-Received: by 2002:a17:906:285a:b0:7ae:127a:cc2e with SMTP id s26-20020a170906285a00b007ae127acc2emr9570808ejc.229.1667565844975; Fri, 04 Nov 2022 05:44:04 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1667565844; cv=none; d=google.com; s=arc-20160816; b=uLShIl0SamlCVnIo/Fw85fWfd0tRjidDIeOXZWlBxaNgFfXq6O+mW5sszZQlAY4B2D 0SJRREBDvh1D/pqup2EisSKdt3vDnHnFeuwHVvOtKqreMnQzWJaE7GgH8xPG6EK7l3C4 IqqJ0fhedewiBlnaqIC3qq4MJAkaC7WEjA2US0+ZCufgEkHnQUEkzzO4rC3x4MhyE3UY UFJJrQGf/K8TTaVwHu3+r9EeMXCddiQNDyh+Bkju76FMN3+4SPz7oBHli8HmwG2ygkmo c6pumX2zsqkOqMvQqSHR0tG371mjErNEurK9kQrZrwAGBOMbcPR7C6DJ/9Msc4jc7BrY h4hA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=QZ0xcUT+4cKR/wNxueAuzQ/myAFZoLhe36cB7EBRMDo=; b=XeSapoOiizyq8xvMoUBVJMx10V7tg6/fgcpi5QSdOGIMBvaSyNCauSJoVn8hFLXyKm Flo2+Kq5aPHN9EMj0L6jOAocqTlSPNTtA4foeeenbIJbpJsDubwEhUVN4aImh92vHdEr vzU0voJeLno7OIVaj0lG0dXptNuXNxeQKQpUjWDVo5OfA0jjTSBKv8JwIT1eTdQkdpGa VsYTM7R0lEtjN6DUmI2cNjwRC2kF+W8+9jmGuMYqO2h0JvqtqmSoDvFPO7cZWsb8Pc3b EGzT41pRe/Gfd+/k5fPLgtgQFW68++Qii8xsVZhtZX/eXjwFfWKhA2V1GbRI1p6Qre3w zRIw== ARC-Authentication-Results: i=1; mx.google.com; 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 j27-20020a170906279b00b007982c5e6ec8si3756422ejc.89.2022.11.04.05.43.40; Fri, 04 Nov 2022 05:44:04 -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; 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 S231838AbiKDMU4 (ORCPT <rfc822;jimliu8233@gmail.com> + 99 others); Fri, 4 Nov 2022 08:20:56 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47084 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231831AbiKDMUy (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 4 Nov 2022 08:20:54 -0400 Received: from frasgout13.his.huawei.com (frasgout13.his.huawei.com [14.137.139.46]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 334892D760; Fri, 4 Nov 2022 05:20:52 -0700 (PDT) Received: from mail02.huawei.com (unknown [172.18.147.227]) by frasgout13.his.huawei.com (SkyGuard) with ESMTP id 4N3fgZ3HXJzB0DDL; Fri, 4 Nov 2022 20:14:18 +0800 (CST) Received: from huaweicloud.com (unknown [10.204.63.22]) by APP1 (Coremail) with SMTP id LxC2BwA3I3CQA2Vjh6Q5AA--.49461S2; Fri, 04 Nov 2022 13:20:38 +0100 (CET) From: Roberto Sassu <roberto.sassu@huaweicloud.com> To: zohar@linux.ibm.com, dmitry.kasatkin@gmail.com, paul@paul-moore.com, jmorris@namei.org, serge@hallyn.com Cc: linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, Roberto Sassu <roberto.sassu@huawei.com>, stable@vger.kernel.org Subject: [PATCH] ima: Make a copy of sig and digest in asymmetric_verify() Date: Fri, 4 Nov 2022 13:20:23 +0100 Message-Id: <20221104122023.1750333-1-roberto.sassu@huaweicloud.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-CM-TRANSID: LxC2BwA3I3CQA2Vjh6Q5AA--.49461S2 X-Coremail-Antispam: 1UD129KBjvJXoWxWFy7KF4fKFWkuF4xKFWDXFb_yoW5Jw15pa 1kKas8KF4UKr4xCFW3Ca1xW3yrWFWrKr47WayfAwn3uFn8Xw4qywn2y3W7Xr98WryxtFW3 trnFqF17Cr1DC3DanT9S1TB71UUUUU7qnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUv2b4IE77IF4wAFF20E14v26r4j6ryUM7CY07I20VC2zVCF04k2 6cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28lY4IEw2IIxxk0rwA2F7IY1VAKz4 vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAFwI0_Jr0_JF4l84ACjcxK6xIIjxv20xvEc7Cj xVAFwI0_Gr0_Cr1l84ACjcxK6I8E87Iv67AKxVW8JVWxJwA2z4x0Y4vEx4A2jsIEc7CjxV AFwI0_Gr0_Gr1UM2AIxVAIcxkEcVAq07x20xvEncxIr21l5I8CrVACY4xI64kE6c02F40E x7xfMcIj6xIIjxv20xvE14v26r106r15McIj6I8E87Iv67AKxVWUJVW8JwAm72CE4IkC6x 0Yz7v_Jr0_Gr1lF7xvr2IYc2Ij64vIr41lFIxGxcIEc7CjxVA2Y2ka0xkIwI1lc7CjxVAa w2AFwI0_GFv_Wryl42xK82IYc2Ij64vIr41l4I8I3I0E4IkC6x0Yz7v_Jr0_Gr1lx2IqxV Aqx4xG67AKxVWUJVWUGwC20s026x8GjcxK67AKxVWUGVWUWwC2zVAF1VAY17CE14v26r1q 6r43MIIYrxkI7VAKI48JMIIF0xvE2Ix0cI8IcVAFwI0_Jr0_JF4lIxAIcVC0I7IYx2IY6x kF7I0E14v26r4j6F4UMIIF0xvE42xK8VAvwI8IcIk0rVW3JVWrJr1lIxAIcVC2z280aVAF wI0_Jr0_Gr1lIxAIcVC2z280aVCY1x0267AKxVW8JVW8JrUvcSsGvfC2KfnxnUUI43ZEXa 7IU8imRUUUUUU== X-CM-SenderInfo: purev21wro2thvvxqx5xdzvxpfor3voofrz/1tbiAQAGBF1jj4URMQAAsX X-CFilter-Loop: Reflected X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_NONE, SPF_PASS 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?1748569523140285680?= X-GMAIL-MSGID: =?utf-8?q?1748569523140285680?= |
Series |
ima: Make a copy of sig and digest in asymmetric_verify()
|
|
Commit Message
Roberto Sassu
Nov. 4, 2022, 12:20 p.m. UTC
From: Roberto Sassu <roberto.sassu@huawei.com> Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear mapping") requires that both the signature and the digest resides in the linear mapping area. However, more recently commit ba14a194a434c ("fork: Add generic vmalloced stack support"), made it possible to move the stack in the vmalloc area, which could make the requirement of the first commit not satisfied anymore. If CONFIG_SG=y and CONFIG_VMAP_STACK=y, the following BUG() is triggered: [ 467.077359] kernel BUG at include/linux/scatterlist.h:163! [ 467.077939] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI [...] [ 467.095225] Call Trace: [ 467.096088] <TASK> [ 467.096928] ? rcu_read_lock_held_common+0xe/0x50 [ 467.097569] ? rcu_read_lock_sched_held+0x13/0x70 [ 467.098123] ? trace_hardirqs_on+0x2c/0xd0 [ 467.098647] ? public_key_verify_signature+0x470/0x470 [ 467.099237] asymmetric_verify+0x14c/0x300 [ 467.099869] evm_verify_hmac+0x245/0x360 [ 467.100391] evm_inode_setattr+0x43/0x190 The failure happens only for the digest, as the pointer comes from the stack, and not for the signature, which instead was allocated by vfs_getxattr_alloc(). Fix this by making a copy of both in asymmetric_verify(), so that the linear mapping requirement is always satisfied, regardless of the caller. Cc: stable@vger.kernel.org # 4.9.x Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support") Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- security/integrity/digsig_asymmetric.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)
Comments
On Fri, 2022-11-04 at 13:20 +0100, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > mapping") requires that both the signature and the digest resides in the > linear mapping area. > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > stack support"), made it possible to move the stack in the vmalloc area, > which could make the requirement of the first commit not satisfied anymore. > > If CONFIG_SG=y and CONFIG_VMAP_STACK=y, the following BUG() is triggered: Hi Mimi did you have the chance to have a look at this patch? Thanks Roberto > [ 467.077359] kernel BUG at include/linux/scatterlist.h:163! > [ 467.077939] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI > > [...] > > [ 467.095225] Call Trace: > [ 467.096088] <TASK> > [ 467.096928] ? rcu_read_lock_held_common+0xe/0x50 > [ 467.097569] ? rcu_read_lock_sched_held+0x13/0x70 > [ 467.098123] ? trace_hardirqs_on+0x2c/0xd0 > [ 467.098647] ? public_key_verify_signature+0x470/0x470 > [ 467.099237] asymmetric_verify+0x14c/0x300 > [ 467.099869] evm_verify_hmac+0x245/0x360 > [ 467.100391] evm_inode_setattr+0x43/0x190 > > The failure happens only for the digest, as the pointer comes from the > stack, and not for the signature, which instead was allocated by > vfs_getxattr_alloc(). > > Fix this by making a copy of both in asymmetric_verify(), so that the > linear mapping requirement is always satisfied, regardless of the caller. > > Cc: stable@vger.kernel.org # 4.9.x > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support") > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > security/integrity/digsig_asymmetric.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c > index 895f4b9ce8c6..635238d5c7fe 100644 > --- a/security/integrity/digsig_asymmetric.c > +++ b/security/integrity/digsig_asymmetric.c > @@ -122,11 +122,26 @@ int asymmetric_verify(struct key *keyring, const char *sig, > goto out; > } > > - pks.digest = (u8 *)data; > + pks.digest = kmemdup(data, datalen, GFP_KERNEL); > + if (!pks.digest) { > + ret = -ENOMEM; > + goto out; > + } > + > pks.digest_size = datalen; > - pks.s = hdr->sig; > + > + pks.s = kmemdup(hdr->sig, siglen, GFP_KERNEL); > + if (!pks.s) { > + kfree(pks.digest); > + ret = -ENOMEM; > + goto out; > + } > + > pks.s_size = siglen; > + > ret = verify_signature(key, &pks); > + kfree(pks.digest); > + kfree(pks.s); > out: > key_put(key); > pr_debug("%s() = %d\n", __func__, ret);
Hi Roberto, On Fri, 2022-11-04 at 13:20 +0100, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > mapping") requires that both the signature and the digest resides in the > linear mapping area. > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > stack support"), made it possible to move the stack in the vmalloc area, > which could make the requirement of the first commit not satisfied anymore. > > If CONFIG_SG=y and CONFIG_VMAP_STACK=y, the following BUG() is triggered: ^CONFIG_DEBUG_SG > > [ 467.077359] kernel BUG at include/linux/scatterlist.h:163! > [ 467.077939] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI > > [...] > > [ 467.095225] Call Trace: > [ 467.096088] <TASK> > [ 467.096928] ? rcu_read_lock_held_common+0xe/0x50 > [ 467.097569] ? rcu_read_lock_sched_held+0x13/0x70 > [ 467.098123] ? trace_hardirqs_on+0x2c/0xd0 > [ 467.098647] ? public_key_verify_signature+0x470/0x470 > [ 467.099237] asymmetric_verify+0x14c/0x300 > [ 467.099869] evm_verify_hmac+0x245/0x360 > [ 467.100391] evm_inode_setattr+0x43/0x190 > > The failure happens only for the digest, as the pointer comes from the > stack, and not for the signature, which instead was allocated by > vfs_getxattr_alloc(). Only after enabling CONFIG_DEBUG_SG does EVM fail. > > Fix this by making a copy of both in asymmetric_verify(), so that the > linear mapping requirement is always satisfied, regardless of the caller. As only EVM is affected, it would make more sense to limit the change to EVM.
On Tue, 2022-11-22 at 14:39 -0500, Mimi Zohar wrote: > Hi Roberto, > > On Fri, 2022-11-04 at 13:20 +0100, Roberto Sassu wrote: > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > mapping") requires that both the signature and the digest resides in the > > linear mapping area. > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > stack support"), made it possible to move the stack in the vmalloc area, > > which could make the requirement of the first commit not satisfied anymore. > > > > If CONFIG_SG=y and CONFIG_VMAP_STACK=y, the following BUG() is triggered: > > ^CONFIG_DEBUG_SG > > > [ 467.077359] kernel BUG at include/linux/scatterlist.h:163! > > [ 467.077939] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI > > > > [...] > > > > [ 467.095225] Call Trace: > > [ 467.096088] <TASK> > > [ 467.096928] ? rcu_read_lock_held_common+0xe/0x50 > > [ 467.097569] ? rcu_read_lock_sched_held+0x13/0x70 > > [ 467.098123] ? trace_hardirqs_on+0x2c/0xd0 > > [ 467.098647] ? public_key_verify_signature+0x470/0x470 > > [ 467.099237] asymmetric_verify+0x14c/0x300 > > [ 467.099869] evm_verify_hmac+0x245/0x360 > > [ 467.100391] evm_inode_setattr+0x43/0x190 > > > > The failure happens only for the digest, as the pointer comes from the > > stack, and not for the signature, which instead was allocated by > > vfs_getxattr_alloc(). > > Only after enabling CONFIG_DEBUG_SG does EVM fail. > > > Fix this by making a copy of both in asymmetric_verify(), so that the > > linear mapping requirement is always satisfied, regardless of the caller. > > As only EVM is affected, it would make more sense to limit the change > to EVM. I found another occurrence: static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, struct evm_ima_xattr_data *xattr_value, int xattr_len, enum integrity_status *status, const char **cause) { [...] rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, (const char *)xattr_value, xattr_len, hash.digest, hash.hdr.length); Should I do two patches? Thanks Roberto
On Wed, 2022-11-23 at 13:56 +0100, Roberto Sassu wrote: > On Tue, 2022-11-22 at 14:39 -0500, Mimi Zohar wrote: > > Hi Roberto, > > > > On Fri, 2022-11-04 at 13:20 +0100, Roberto Sassu wrote: > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > > mapping") requires that both the signature and the digest resides in the > > > linear mapping area. > > > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > > stack support"), made it possible to move the stack in the vmalloc area, > > > which could make the requirement of the first commit not satisfied anymore. > > > > > > If CONFIG_SG=y and CONFIG_VMAP_STACK=y, the following BUG() is triggered: > > > > ^CONFIG_DEBUG_SG > > > > > [ 467.077359] kernel BUG at include/linux/scatterlist.h:163! > > > [ 467.077939] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI > > > > > > [...] > > > > > > [ 467.095225] Call Trace: > > > [ 467.096088] <TASK> > > > [ 467.096928] ? rcu_read_lock_held_common+0xe/0x50 > > > [ 467.097569] ? rcu_read_lock_sched_held+0x13/0x70 > > > [ 467.098123] ? trace_hardirqs_on+0x2c/0xd0 > > > [ 467.098647] ? public_key_verify_signature+0x470/0x470 > > > [ 467.099237] asymmetric_verify+0x14c/0x300 > > > [ 467.099869] evm_verify_hmac+0x245/0x360 > > > [ 467.100391] evm_inode_setattr+0x43/0x190 > > > > > > The failure happens only for the digest, as the pointer comes from the > > > stack, and not for the signature, which instead was allocated by > > > vfs_getxattr_alloc(). > > > > Only after enabling CONFIG_DEBUG_SG does EVM fail. > > > > > Fix this by making a copy of both in asymmetric_verify(), so that the > > > linear mapping requirement is always satisfied, regardless of the caller. > > > > As only EVM is affected, it would make more sense to limit the change > > to EVM. > > I found another occurrence: > > static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, > struct evm_ima_xattr_data *xattr_value, int xattr_len, > enum integrity_status *status, const char **cause) > { > > [...] > > rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > (const char *)xattr_value, > xattr_len, hash.digest, > hash.hdr.length); > > Should I do two patches? I'm just not getting it. Why did you enable CONFIG_DEBUG_SIG? Were you testing random kernel configs? Are you actually seeing signature verifications errors without it enabled? Or is it causing other problems? Is the "BUG_ON" still needed? If you're going to fix the EVM and IMA callers, then make them separate patches.
On Wed, 2022-11-23 at 08:40 -0500, Mimi Zohar wrote: > On Wed, 2022-11-23 at 13:56 +0100, Roberto Sassu wrote: > > On Tue, 2022-11-22 at 14:39 -0500, Mimi Zohar wrote: > > > Hi Roberto, > > > > > > On Fri, 2022-11-04 at 13:20 +0100, Roberto Sassu wrote: > > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > > > mapping") requires that both the signature and the digest resides in the > > > > linear mapping area. > > > > > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > > > stack support"), made it possible to move the stack in the vmalloc area, > > > > which could make the requirement of the first commit not satisfied anymore. > > > > > > > > If CONFIG_SG=y and CONFIG_VMAP_STACK=y, the following BUG() is triggered: > > > > > > ^CONFIG_DEBUG_SG > > > > > > > [ 467.077359] kernel BUG at include/linux/scatterlist.h:163! > > > > [ 467.077939] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI > > > > > > > > [...] > > > > > > > > [ 467.095225] Call Trace: > > > > [ 467.096088] <TASK> > > > > [ 467.096928] ? rcu_read_lock_held_common+0xe/0x50 > > > > [ 467.097569] ? rcu_read_lock_sched_held+0x13/0x70 > > > > [ 467.098123] ? trace_hardirqs_on+0x2c/0xd0 > > > > [ 467.098647] ? public_key_verify_signature+0x470/0x470 > > > > [ 467.099237] asymmetric_verify+0x14c/0x300 > > > > [ 467.099869] evm_verify_hmac+0x245/0x360 > > > > [ 467.100391] evm_inode_setattr+0x43/0x190 > > > > > > > > The failure happens only for the digest, as the pointer comes from the > > > > stack, and not for the signature, which instead was allocated by > > > > vfs_getxattr_alloc(). > > > > > > Only after enabling CONFIG_DEBUG_SG does EVM fail. > > > > > > > Fix this by making a copy of both in asymmetric_verify(), so that the > > > > linear mapping requirement is always satisfied, regardless of the caller. > > > > > > As only EVM is affected, it would make more sense to limit the change > > > to EVM. > > > > I found another occurrence: > > > > static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, > > struct evm_ima_xattr_data *xattr_value, int xattr_len, > > enum integrity_status *status, const char **cause) > > { > > > > [...] > > > > rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > > (const char *)xattr_value, > > xattr_len, hash.digest, > > hash.hdr.length); > > > > Should I do two patches? > > I'm just not getting it. Why did you enable CONFIG_DEBUG_SIG? Were > you testing random kernel configs? Are you actually seeing signature > verifications errors without it enabled? Or is it causing other > problems? Is the "BUG_ON" still needed? When I test patches, I tend to enable more debugging options. To be honest, I didn't check if there is any issue without enabling CONFIG_DEBUG_SG. I thought that if there is a linear mapping requirement, that should be satisfied regardless of whether the debugging option is enabled or not. + Rusty, Jens for explanations. > If you're going to fix the EVM and IMA callers, then make them separate > patches. Ok. Thanks Roberto
On Wed, 2022-11-23 at 14:49 +0100, Roberto Sassu wrote: > On Wed, 2022-11-23 at 08:40 -0500, Mimi Zohar wrote: > > On Wed, 2022-11-23 at 13:56 +0100, Roberto Sassu wrote: > > > On Tue, 2022-11-22 at 14:39 -0500, Mimi Zohar wrote: > > > > Hi Roberto, > > > > > > > > On Fri, 2022-11-04 at 13:20 +0100, Roberto Sassu wrote: > > > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > > > > mapping") requires that both the signature and the digest resides in the > > > > > linear mapping area. > > > > > > > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > > > > stack support"), made it possible to move the stack in the vmalloc area, > > > > > which could make the requirement of the first commit not satisfied anymore. > > > > > > > > > > If CONFIG_SG=y and CONFIG_VMAP_STACK=y, the following BUG() is triggered: > > > > > > > > ^CONFIG_DEBUG_SG > > > > > > > > > [ 467.077359] kernel BUG at include/linux/scatterlist.h:163! > > > > > [ 467.077939] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI > > > > > > > > > > [...] > > > > > > > > > > [ 467.095225] Call Trace: > > > > > [ 467.096088] <TASK> > > > > > [ 467.096928] ? rcu_read_lock_held_common+0xe/0x50 > > > > > [ 467.097569] ? rcu_read_lock_sched_held+0x13/0x70 > > > > > [ 467.098123] ? trace_hardirqs_on+0x2c/0xd0 > > > > > [ 467.098647] ? public_key_verify_signature+0x470/0x470 > > > > > [ 467.099237] asymmetric_verify+0x14c/0x300 > > > > > [ 467.099869] evm_verify_hmac+0x245/0x360 > > > > > [ 467.100391] evm_inode_setattr+0x43/0x190 > > > > > > > > > > The failure happens only for the digest, as the pointer comes from the > > > > > stack, and not for the signature, which instead was allocated by > > > > > vfs_getxattr_alloc(). > > > > > > > > Only after enabling CONFIG_DEBUG_SG does EVM fail. > > > > > > > > > Fix this by making a copy of both in asymmetric_verify(), so that the > > > > > linear mapping requirement is always satisfied, regardless of the caller. > > > > > > > > As only EVM is affected, it would make more sense to limit the change > > > > to EVM. > > > > > > I found another occurrence: > > > > > > static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, > > > struct evm_ima_xattr_data *xattr_value, int xattr_len, > > > enum integrity_status *status, const char **cause) > > > { > > > > > > [...] > > > > > > rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > > > (const char *)xattr_value, > > > xattr_len, hash.digest, > > > hash.hdr.length); > > > > > > Should I do two patches? > > > > I'm just not getting it. Why did you enable CONFIG_DEBUG_SIG? Were > > you testing random kernel configs? Are you actually seeing signature > > verifications errors without it enabled? Or is it causing other > > problems? Is the "BUG_ON" still needed? > > When I test patches, I tend to enable more debugging options. > > To be honest, I didn't check if there is any issue without enabling > CONFIG_DEBUG_SG. I thought that if there is a linear mapping > requirement, that should be satisfied regardless of whether the > debugging option is enabled or not. > > + Rusty, Jens for explanations. Trying to answer the question, with the help of an old discussion: https://groups.google.com/g/linux.kernel/c/dpIoiY_qSGc sg_set_buf() calls virt_to_page() to get the page to start from. But if the buffer spans in two pages, that would not work in the vmalloc area, since there is no guarantee that the next page is adjiacent. For small areas, much smaller than the page size, it is unlikely that the situation above would happen. So, integrity_digsig_verify() will likely succeeed. Although it is possible that it fails if there are data in the next page. Roberto
On Wed, 2022-11-30 at 15:41 +0100, Roberto Sassu wrote: > On Wed, 2022-11-23 at 14:49 +0100, Roberto Sassu wrote: > > On Wed, 2022-11-23 at 08:40 -0500, Mimi Zohar wrote: > > > On Wed, 2022-11-23 at 13:56 +0100, Roberto Sassu wrote: > > > > On Tue, 2022-11-22 at 14:39 -0500, Mimi Zohar wrote: > > > > > Hi Roberto, > > > > > > > > > > On Fri, 2022-11-04 at 13:20 +0100, Roberto Sassu wrote: > > > > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > > > > > mapping") requires that both the signature and the digest resides in the > > > > > > linear mapping area. > > > > > > > > > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > > > > > stack support"), made it possible to move the stack in the vmalloc area, > > > > > > which could make the requirement of the first commit not satisfied anymore. > > > > > > > > > > > > If CONFIG_SG=y and CONFIG_VMAP_STACK=y, the following BUG() is triggered: > > > > > > > > > > ^CONFIG_DEBUG_SG > > > > > > > > > > > [ 467.077359] kernel BUG at include/linux/scatterlist.h:163! > > > > > > [ 467.077939] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI > > > > > > > > > > > > [...] > > > > > > > > > > > > [ 467.095225] Call Trace: > > > > > > [ 467.096088] <TASK> > > > > > > [ 467.096928] ? rcu_read_lock_held_common+0xe/0x50 > > > > > > [ 467.097569] ? rcu_read_lock_sched_held+0x13/0x70 > > > > > > [ 467.098123] ? trace_hardirqs_on+0x2c/0xd0 > > > > > > [ 467.098647] ? public_key_verify_signature+0x470/0x470 > > > > > > [ 467.099237] asymmetric_verify+0x14c/0x300 > > > > > > [ 467.099869] evm_verify_hmac+0x245/0x360 > > > > > > [ 467.100391] evm_inode_setattr+0x43/0x190 > > > > > > > > > > > > The failure happens only for the digest, as the pointer comes from the > > > > > > stack, and not for the signature, which instead was allocated by > > > > > > vfs_getxattr_alloc(). > > > > > > > > > > Only after enabling CONFIG_DEBUG_SG does EVM fail. > > > > > > > > > > > Fix this by making a copy of both in asymmetric_verify(), so that the > > > > > > linear mapping requirement is always satisfied, regardless of the caller. > > > > > > > > > > As only EVM is affected, it would make more sense to limit the change > > > > > to EVM. > > > > > > > > I found another occurrence: > > > > > > > > static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, > > > > struct evm_ima_xattr_data *xattr_value, int xattr_len, > > > > enum integrity_status *status, const char **cause) > > > > { > > > > > > > > [...] > > > > > > > > rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > > > > (const char *)xattr_value, > > > > xattr_len, hash.digest, > > > > hash.hdr.length); > > > > > > > > Should I do two patches? > > > > > > I'm just not getting it. Why did you enable CONFIG_DEBUG_SIG? Were > > > you testing random kernel configs? Are you actually seeing signature > > > verifications errors without it enabled? Or is it causing other > > > problems? Is the "BUG_ON" still needed? > > > > When I test patches, I tend to enable more debugging options. > > > > To be honest, I didn't check if there is any issue without enabling > > CONFIG_DEBUG_SG. I thought that if there is a linear mapping > > requirement, that should be satisfied regardless of whether the > > debugging option is enabled or not. > > > > + Rusty, Jens for explanations. > > Trying to answer the question, with the help of an old discussion: > > https://groups.google.com/g/linux.kernel/c/dpIoiY_qSGc > > sg_set_buf() calls virt_to_page() to get the page to start from. But if > the buffer spans in two pages, that would not work in the vmalloc area, > since there is no guarantee that the next page is adjiacent. > > For small areas, much smaller than the page size, it is unlikely that > the situation above would happen. So, integrity_digsig_verify() will > likely succeeed. Although it is possible that it fails if there are > data in the next page. Thanks, Roberto. Confirmed that as the patch description indicates, without CONFIG_VMAP_STACK configured and with CONFIG_DEBUG_SG enabled there isn't a bug. Does it make sense to limit this change to just CONFIG_VMAP_STACK?
On Wed, 2022-11-30 at 11:22 -0500, Mimi Zohar wrote: > On Wed, 2022-11-30 at 15:41 +0100, Roberto Sassu wrote: > > On Wed, 2022-11-23 at 14:49 +0100, Roberto Sassu wrote: > > > On Wed, 2022-11-23 at 08:40 -0500, Mimi Zohar wrote: > > > > On Wed, 2022-11-23 at 13:56 +0100, Roberto Sassu wrote: > > > > > On Tue, 2022-11-22 at 14:39 -0500, Mimi Zohar wrote: > > > > > > Hi Roberto, > > > > > > > > > > > > On Fri, 2022-11-04 at 13:20 +0100, Roberto Sassu wrote: > > > > > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > > > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > > > > > > mapping") requires that both the signature and the digest resides in the > > > > > > > linear mapping area. > > > > > > > > > > > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > > > > > > stack support"), made it possible to move the stack in the vmalloc area, > > > > > > > which could make the requirement of the first commit not satisfied anymore. > > > > > > > > > > > > > > If CONFIG_SG=y and CONFIG_VMAP_STACK=y, the following BUG() is triggered: > > > > > > > > > > > > ^CONFIG_DEBUG_SG > > > > > > > > > > > > > [ 467.077359] kernel BUG at include/linux/scatterlist.h:163! > > > > > > > [ 467.077939] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > [ 467.095225] Call Trace: > > > > > > > [ 467.096088] <TASK> > > > > > > > [ 467.096928] ? rcu_read_lock_held_common+0xe/0x50 > > > > > > > [ 467.097569] ? rcu_read_lock_sched_held+0x13/0x70 > > > > > > > [ 467.098123] ? trace_hardirqs_on+0x2c/0xd0 > > > > > > > [ 467.098647] ? public_key_verify_signature+0x470/0x470 > > > > > > > [ 467.099237] asymmetric_verify+0x14c/0x300 > > > > > > > [ 467.099869] evm_verify_hmac+0x245/0x360 > > > > > > > [ 467.100391] evm_inode_setattr+0x43/0x190 > > > > > > > > > > > > > > The failure happens only for the digest, as the pointer comes from the > > > > > > > stack, and not for the signature, which instead was allocated by > > > > > > > vfs_getxattr_alloc(). > > > > > > > > > > > > Only after enabling CONFIG_DEBUG_SG does EVM fail. > > > > > > > > > > > > > Fix this by making a copy of both in asymmetric_verify(), so that the > > > > > > > linear mapping requirement is always satisfied, regardless of the caller. > > > > > > > > > > > > As only EVM is affected, it would make more sense to limit the change > > > > > > to EVM. > > > > > > > > > > I found another occurrence: > > > > > > > > > > static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, > > > > > struct evm_ima_xattr_data *xattr_value, int xattr_len, > > > > > enum integrity_status *status, const char **cause) > > > > > { > > > > > > > > > > [...] > > > > > > > > > > rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > > > > > (const char *)xattr_value, > > > > > xattr_len, hash.digest, > > > > > hash.hdr.length); > > > > > > > > > > Should I do two patches? > > > > > > > > I'm just not getting it. Why did you enable CONFIG_DEBUG_SIG? Were > > > > you testing random kernel configs? Are you actually seeing signature > > > > verifications errors without it enabled? Or is it causing other > > > > problems? Is the "BUG_ON" still needed? > > > > > > When I test patches, I tend to enable more debugging options. > > > > > > To be honest, I didn't check if there is any issue without enabling > > > CONFIG_DEBUG_SG. I thought that if there is a linear mapping > > > requirement, that should be satisfied regardless of whether the > > > debugging option is enabled or not. > > > > > > + Rusty, Jens for explanations. > > > > Trying to answer the question, with the help of an old discussion: > > > > https://groups.google.com/g/linux.kernel/c/dpIoiY_qSGc > > > > sg_set_buf() calls virt_to_page() to get the page to start from. But if > > the buffer spans in two pages, that would not work in the vmalloc area, > > since there is no guarantee that the next page is adjiacent. > > > > For small areas, much smaller than the page size, it is unlikely that > > the situation above would happen. So, integrity_digsig_verify() will > > likely succeeed. Although it is possible that it fails if there are > > data in the next page. > > Thanks, Roberto. Confirmed that as the patch description indicates, > without CONFIG_VMAP_STACK configured and with CONFIG_DEBUG_SG enabled > there isn't a bug. Does it make sense to limit this change to just > CONFIG_VMAP_STACK? Yes, I agree. Roberto
diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c index 895f4b9ce8c6..635238d5c7fe 100644 --- a/security/integrity/digsig_asymmetric.c +++ b/security/integrity/digsig_asymmetric.c @@ -122,11 +122,26 @@ int asymmetric_verify(struct key *keyring, const char *sig, goto out; } - pks.digest = (u8 *)data; + pks.digest = kmemdup(data, datalen, GFP_KERNEL); + if (!pks.digest) { + ret = -ENOMEM; + goto out; + } + pks.digest_size = datalen; - pks.s = hdr->sig; + + pks.s = kmemdup(hdr->sig, siglen, GFP_KERNEL); + if (!pks.s) { + kfree(pks.digest); + ret = -ENOMEM; + goto out; + } + pks.s_size = siglen; + ret = verify_signature(key, &pks); + kfree(pks.digest); + kfree(pks.s); out: key_put(key); pr_debug("%s() = %d\n", __func__, ret);