From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932416AbdJZXNr (ORCPT ); Thu, 26 Oct 2017 19:13:47 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:54102 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932402AbdJZXNm (ORCPT ); Thu, 26 Oct 2017 19:13:42 -0400 Subject: Re: [PATCH v5 12/18] MODSIGN: Export module signature definitions From: Mimi Zohar To: Thiago Jung Bauermann 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" Date: Thu, 26 Oct 2017 19:13:30 -0400 In-Reply-To: <87po99wmpq.fsf@linux.vnet.ibm.com> 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> <87po99wmpq.fsf@linux.vnet.ibm.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.5 (3.20.5-1.fc24) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 17102623-0012-0000-0000-0000058660CB X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17102623-0013-0000-0000-00001900DEA3 Message-Id: <1509059610.5886.145.camel@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=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1710260293 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2017-10-26 at 20:47 -0200, Thiago Jung Bauermann wrote: > 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? No, the original code was fine.