From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 1E0B0C433F5 for ; Tue, 22 Mar 2022 07:01:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=5SHSWSKpmFty4rRvAyVEckswgzhjSeSlusS7Ylnls+c=; b=LH4stDOAZ8iJrrfpGmmlLQ/H+J ygi2UGX56Nl9CCGBCRR+oSEyaiRBOPxTFqLm7VHrly0ejgzQxjNeHpkpXPqGrrMcAsstEkCmpzoSZ Q5/fHRtf1miJ6ZTbZqe+lVyPNPgM7RDgXfdmIteTBV8oQ6zP5rpC4YE5lXevDZpHKaftedP4ifB6S f1xNyWCET0ulEeV8fQ/iz5XW9Kpws23IiDyHvBhH6q8N8jwIQcOq/Cng3DsHwZVGjAxybYdP6Ljb+ jG1XSXXoY6MOgNpIJFhMVP7cTaqCmywhBcxo2HM2kM77JEk5eYQeNIlfSDAByqL8amoSrD2HeTr+p 734IfPNw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nWYVL-00AECz-KS; Tue, 22 Mar 2022 07:00:31 +0000 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nWYVH-00AECE-UR for linux-arm-kernel@lists.infradead.org; Tue, 22 Mar 2022 07:00:29 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1647932427; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Wcf2SYtc4GFd9+9KclXmq9oA4rCndyWVsVQSDgBqSmo=; b=N8Awi67A9TCG6XJ+C/CeqjqNz1Qe8aUvhUU7ka1sURo2smAT6WIGJTNZfsDDy+S4CWyniB VVTnyxYlN966sXCX1cMvPls22hyWsSgnL1M+3VjRG/H3H42cweVXGeYEbC2dwueeb2eTdv 6zNAzF/XNvGiGgi7tl5itjh+c6HIhIg= Received: from mail-pf1-f198.google.com (mail-pf1-f198.google.com [209.85.210.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-30-LfhqOoJPPsmUw7w7av6sJA-1; Tue, 22 Mar 2022 03:00:23 -0400 X-MC-Unique: LfhqOoJPPsmUw7w7av6sJA-1 Received: by mail-pf1-f198.google.com with SMTP id a23-20020aa794b7000000b004f6a3ac7a87so10930011pfl.23 for ; Tue, 22 Mar 2022 00:00:23 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=Wcf2SYtc4GFd9+9KclXmq9oA4rCndyWVsVQSDgBqSmo=; b=S9LbX3GpZW0mM9rd+H5YHMYyW9fWEGZ542OoWs90y/BdCsocLpBoZwkqkAwddZExwt 3doCFAbDdxx8Jmynm2x1ovPIch9mOVCgIzgBz3Etugx15qbIUpH/HghEQoKGumqg4AvY 74LQg8kHKYP1pBp78M5He/qSDS5OKIXC7XlJxEkizM1VXqFN4YjCEtONcZFFZO0Oq+Cr dkrgHwbBNzDY+zJ2woG0riQ8kB2/wsJebjnOahYCVrWhQ6QVDUrP67Z6OmyHYTyY4bmA DuuFh6ylk5HL4ItVQSp6WmHlFMNxRFOX1fThksVETxqnO96BBiNrbLCU6nkwdzZjFlQ3 p6Xw== X-Gm-Message-State: AOAM5331wsye+f85LztAZ2vGoaRioFaN8fKsHtW/yA0FTgCCJyoj9zFo ut6i+vaMNiZYwqiyOPjGndao4uZsO+njoXelVsir6Hx6DqWy0Xi9e19ydj9OqENgdIvaSCNEmXd y2x3/54YY1OJIm1gCCn2ZbdZvToepnUZhOx8= X-Received: by 2002:a63:614:0:b0:383:c70e:8613 with SMTP id 20-20020a630614000000b00383c70e8613mr1785661pgg.85.1647932422431; Tue, 22 Mar 2022 00:00:22 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxDLAJGdDx1DX9QdGgd64bEDHVrgInDmENiGDd/21zUmQiwkY+OpHCg755hm6FNPYsJQG4L+g== X-Received: by 2002:a63:614:0:b0:383:c70e:8613 with SMTP id 20-20020a630614000000b00383c70e8613mr1785643pgg.85.1647932422091; Tue, 22 Mar 2022 00:00:22 -0700 (PDT) Received: from localhost ([240e:3a1:2e5:800:f995:6136:f760:a3d0]) by smtp.gmail.com with ESMTPSA id b20-20020a17090a991400b001befe07ae5csm1533696pjp.22.2022.03.22.00.00.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 22 Mar 2022 00:00:21 -0700 (PDT) Date: Tue, 22 Mar 2022 14:57:59 +0800 From: Coiby Xu To: Baoquan He Cc: kexec@lists.infradead.org, linux-arm-kernel@lists.infradead.org, Dave Young , Will Deacon , "Eric W . Biederman" , open list Subject: Re: [PATCH v4 1/3] kexec: clean up arch_kexec_kernel_verify_sig Message-ID: <20220322065759.2v4o3qekkc3tjroy@Rk> References: <20220318094101.274950-1-coxu@redhat.com> <20220318094101.274950-2-coxu@redhat.com> <20220322025912.r2ahc2ztx3npt7av@Rk> MIME-Version: 1.0 In-Reply-To: Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=coxu@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220322_000028_099026_8142D949 X-CRM114-Status: GOOD ( 36.08 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Mar 22, 2022 at 11:13:20AM +0800, Baoquan He wrote: >On 03/22/22 at 10:59am, Coiby Xu wrote: >> On Mon, Mar 21, 2022 at 12:21:33PM +0800, Baoquan He wrote: >> > On 03/18/22 at 05:40pm, Coiby Xu wrote: >> > > Commit 9ec4ecef0af7 ("kexec_file,x86,powerpc: factor out kexec_file_ops >> > > functions") allows implementing the arch-specific implementation of kernel >> > > image verification in kexec_file_ops->verify_sig. Currently, there is no >> > >> > Looking back at the old commit 9ec4ecef0af7, it mistakenly added a >> > generic arch_kexec_kernel_verify_sig() which is marked as __weak, >> > and expects any architecture will add a arch specified version if needed. >> > In fact those arch specified difference has been removed by wrapping >> > them into each architecture's own struct kexec_file_ops methods. Means >> > in the commit, the generic arch_kexec_kernel_verify_sig() is unnecessary >> > at all. >> >> Thanks for looking at commit 9ec4ecef0af7 for me! >> >> Although commit 9ec4ecef0af7 added some code in arch_kexec_kernel_verify_sig >> so kexec_file_ops->verify_sig can be called, this commit doesn't add __weak >> arch_kexec_kernel_verify_sig itself. And kexec_file_ops isn't supposed >> to replace arch-specific implementation using __weak considering s390 and x86 >> still make use of __weak to implement its own version of >> arch_kexec_apply_relocations_add. How about the commit message as >> follows? > >Yes, arch_kexec_apply_relocations_add has its different version on >arches. But arch_kexec_kernel_verify_sig() is different. There's a >specific method for that, ->verify_sig(). > >struct kexec_file_ops { > kexec_probe_t *probe; > kexec_load_t *load; > kexec_cleanup_t *cleanup; >#ifdef CONFIG_KEXEC_SIG > kexec_verify_sig_t *verify_sig; >#endif >}; > Thanks for the explanation! This example of arch_kexec_apply_relocations_add is indeed not good and don't illustrate my point. My point is we can't say commit 9ec4ecef0af7 made a mistake since it's not this commit that introduced "__weak arch_kexec_kernel_verify_sig" and I don't think its motivation was to replace __weak with kexec_file_ops. Currently we still have "__weak arch_kimage_file_post_load_cleanup" and kexec_file_ops->cleanup. >> >> Currently this no arch-specific implementation of >> arch_kexec_kernel_verify_sig. Even if we want to add an implementation >> for an architecture in the future, we can simply use "(struct >> kexec_file_ops*)->verify_sig". So clean it up. > >That is also fine. I think it's better to put the above in if we have >checked the old commit. Anyway, please take the sentences which comforts >you more. And there's grammer mistake, please use 'Currently there is >not' to replace. Thanks for catching the mistake! > >> > >> > Now, you clean up that uncessary function with code change. >> > >> > I think description telling above analysis could be clearer. >> > >> > > arch-specific implementation of arch_kexec_kernel_verify_sig. So clean it >> > > up. >> > > >> > > Suggested-by: Eric W. Biederman >> > > Signed-off-by: Coiby Xu >> > > --- >> > > include/linux/kexec.h | 4 ---- >> > > kernel/kexec_file.c | 34 +++++++++++++--------------------- >> > > 2 files changed, 13 insertions(+), 25 deletions(-) >> > > >> > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h >> > > index 0c994ae37729..755fed183224 100644 >> > > --- a/include/linux/kexec.h >> > > +++ b/include/linux/kexec.h >> > > @@ -196,10 +196,6 @@ int arch_kexec_apply_relocations(struct purgatory_info *pi, >> > > const Elf_Shdr *relsec, >> > > const Elf_Shdr *symtab); >> > > int arch_kimage_file_post_load_cleanup(struct kimage *image); >> > > -#ifdef CONFIG_KEXEC_SIG >> > > -int arch_kexec_kernel_verify_sig(struct kimage *image, void *buf, >> > > - unsigned long buf_len); >> > > -#endif >> > > int arch_kexec_locate_mem_hole(struct kexec_buf *kbuf); >> > > >> > > extern int kexec_add_buffer(struct kexec_buf *kbuf); >> > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c >> > > index 8347fc158d2b..3720435807eb 100644 >> > > --- a/kernel/kexec_file.c >> > > +++ b/kernel/kexec_file.c >> > > @@ -89,25 +89,6 @@ int __weak arch_kimage_file_post_load_cleanup(struct kimage *image) >> > > return kexec_image_post_load_cleanup_default(image); >> > > } >> > > >> > > -#ifdef CONFIG_KEXEC_SIG >> > > -static int kexec_image_verify_sig_default(struct kimage *image, void *buf, >> > > - unsigned long buf_len) >> > > -{ >> > > - if (!image->fops || !image->fops->verify_sig) { >> > > - pr_debug("kernel loader does not support signature verification.\n"); >> > > - return -EKEYREJECTED; >> > > - } >> > > - >> > > - return image->fops->verify_sig(buf, buf_len); >> > > -} >> > > - >> > > -int __weak arch_kexec_kernel_verify_sig(struct kimage *image, void *buf, >> > > - unsigned long buf_len) >> > > -{ >> > > - return kexec_image_verify_sig_default(image, buf, buf_len); >> > > -} >> > > -#endif >> > > - >> > > /* >> > > * arch_kexec_apply_relocations_add - apply relocations of type RELA >> > > * @pi: Purgatory to be relocated. >> > > @@ -184,13 +165,24 @@ void kimage_file_post_load_cleanup(struct kimage *image) >> > > } >> > > >> > > #ifdef CONFIG_KEXEC_SIG >> > > +static int kexec_image_verify_sig(struct kimage *image, void *buf, >> > > + unsigned long buf_len) >> > > +{ >> > > + if (!image->fops || !image->fops->verify_sig) { >> > > + pr_debug("kernel loader does not support signature verification.\n"); >> > > + return -EKEYREJECTED; >> > > + } >> > > + >> > > + return image->fops->verify_sig(buf, buf_len); >> > > +} >> > > + >> > > static int >> > > kimage_validate_signature(struct kimage *image) >> > > { >> > > int ret; >> > > >> > > - ret = arch_kexec_kernel_verify_sig(image, image->kernel_buf, >> > > - image->kernel_buf_len); >> > > + ret = kexec_image_verify_sig(image, image->kernel_buf, >> > > + image->kernel_buf_len); >> > > if (ret) { >> > > >> > > if (IS_ENABLED(CONFIG_KEXEC_SIG_FORCE)) { >> > > -- >> > > 2.34.1 >> > > >> > >> >> -- >> Best regards, >> Coiby >> > -- Best regards, Coiby _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel