From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932398AbdJZWrp (ORCPT ); Thu, 26 Oct 2017 18:47:45 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:38044 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751457AbdJZWrm (ORCPT ); Thu, 26 Oct 2017 18:47:42 -0400 References: <20171018005331.2688-1-bauerman@linux.vnet.ibm.com> <20171018005331.2688-13-bauerman@linux.vnet.ibm.com> <1509048728.5886.112.camel@linux.vnet.ibm.com> From: Thiago Jung Bauermann To: Mimi Zohar Cc: linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, keyrings@vger.kernel.org, linux-crypto@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, Dmitry Kasatkin , James Morris , "Serge E. Hallyn" , David Howells , David Woodhouse , Jessica Yu , Rusty Russell , Herbert Xu , "David S. Miller" , "AKASHI\, Takahiro" Subject: Re: [PATCH v5 12/18] MODSIGN: Export module signature definitions In-reply-to: <1509048728.5886.112.camel@linux.vnet.ibm.com> Date: Thu, 26 Oct 2017 20:47:29 -0200 MIME-Version: 1.0 Content-Type: text/plain X-TM-AS-GCONF: 00 x-cbid: 17102622-0008-0000-0000-00000296DFA3 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007957; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000239; SDB=6.00936927; UDB=6.00472181; IPR=6.00717189; BA=6.00005660; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00017731; XFM=3.00000015; UTC=2017-10-26 22:47:40 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17102622-0009-0000-0000-00003720EF6D Message-Id: <87po99wmpq.fsf@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-10-26_10:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=1 phishscore=0 bulkscore=0 spamscore=0 clxscore=1011 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1710260288 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Mimi Zohar writes: > On Tue, 2017-10-17 at 22:53 -0200, Thiago Jung Bauermann wrote: >> IMA will use the module_signature format for append signatures, so export >> the relevant definitions and factor out the code which verifies that the >> appended signature trailer is valid. >> >> Also, create a CONFIG_MODULE_SIG_FORMAT option so that IMA can select it >> and be able to use validate_module_signature without having to depend on >> CONFIG_MODULE_SIG. >> >> Signed-off-by: Thiago Jung Bauermann > > Reviewed-by: Mimi Zohar > > One minor comment below... Thanks! >> diff --git a/kernel/module_signing.c b/kernel/module_signing.c >> index 937c844bee4a..204c60d4cc9f 100644 >> --- a/kernel/module_signing.c >> +++ b/kernel/module_signing.c >> @@ -11,36 +11,38 @@ >> >> #include >> #include >> +#include >> #include >> #include >> #include >> #include "module-internal.h" >> >> -enum pkey_id_type { >> - PKEY_ID_PGP, /* OpenPGP generated key ID */ >> - PKEY_ID_X509, /* X.509 arbitrary subjectKeyIdentifier */ >> - PKEY_ID_PKCS7, /* Signature in PKCS#7 message */ >> -}; >> - >> -/* >> - * Module signature information block. >> - * >> - * The constituents of the signature section are, in order: >> +/** >> + * validate_module_sig - validate that the given signature is sane >> * >> - * - Signer's name >> - * - Key identifier >> - * - Signature data >> - * - Information block >> + * @ms: Signature to validate. >> + * @file_len: Size of the file to which @ms is appended. >> */ >> -struct module_signature { >> - u8 algo; /* Public-key crypto algorithm [0] */ >> - u8 hash; /* Digest algorithm [0] */ >> - u8 id_type; /* Key identifier type [PKEY_ID_PKCS7] */ >> - u8 signer_len; /* Length of signer's name [0] */ >> - u8 key_id_len; /* Length of key identifier [0] */ >> - u8 __pad[3]; >> - __be32 sig_len; /* Length of signature data */ >> -}; >> +int validate_module_sig(const struct module_signature *ms, size_t file_len) >> +{ >> + if (be32_to_cpu(ms->sig_len) >= file_len - sizeof(*ms)) >> + return -EBADMSG; >> + else if (ms->id_type != PKEY_ID_PKCS7) { >> + pr_err("Module is not signed with expected PKCS#7 message\n"); >> + return -ENOPKG; >> + } else if (ms->algo != 0 || >> + ms->hash != 0 || >> + ms->signer_len != 0 || >> + ms->key_id_len != 0 || >> + ms->__pad[0] != 0 || >> + ms->__pad[1] != 0 || >> + ms->__pad[2] != 0) { >> + pr_err("PKCS#7 signature info has unexpected non-zero params\n"); >> + return -EBADMSG; >> + } >> + > > When moving code from one place to another, it's easier to review when > there aren't code changes as well. In this case, the original code > doesn't have "else clauses". Indeed. I changed the code back to using separate if clauses, making only the changes that are required for the refactoring. > Here some of the if/then/else clauses > have braces others don't. There shouldn't be a mixture. Does this still apply when the if clauses are separate as in the original code? Should the first if still have braces? -- Thiago Jung Bauermann IBM Linux Technology Center