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 X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8EC8EC433ED for ; Wed, 14 Apr 2021 14:07:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6E6BE61249 for ; Wed, 14 Apr 2021 14:07:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1346555AbhDNOH6 (ORCPT ); Wed, 14 Apr 2021 10:07:58 -0400 Received: from mail.hallyn.com ([178.63.66.53]:39066 "EHLO mail.hallyn.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S243063AbhDNOH6 (ORCPT ); Wed, 14 Apr 2021 10:07:58 -0400 Received: by mail.hallyn.com (Postfix, from userid 1001) id 54277A30; Wed, 14 Apr 2021 09:07:34 -0500 (CDT) Date: Wed, 14 Apr 2021 09:07:34 -0500 From: "Serge E. Hallyn" To: Dan Carpenter Cc: Sumit Garg , James Bottomley , Jarkko Sakkinen , Mimi Zohar , David Howells , James Morris , "Serge E. Hallyn" , linux-integrity@vger.kernel.org, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, kernel-janitors@vger.kernel.org Subject: Re: [PATCH] KEYS: trusted: fix a couple error pointer dereferences Message-ID: <20210414140734.GB11180@mail.hallyn.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) Precedence: bulk List-ID: X-Mailing-List: linux-integrity@vger.kernel.org On Wed, Apr 14, 2021 at 09:08:58AM +0300, Dan Carpenter wrote: > If registering "reg_shm_out" fails, then it is an error pointer and the > error handling will call tee_shm_free(reg_shm_out) which leads to an > error pointer dereference and an Oops. > > I've re-arranged it so we only free things that have been allocated > successfully. > > Fixes: 6dd95e650c8a ("KEYS: trusted: Introduce TEE based Trusted Keys") > Signed-off-by: Dan Carpenter > --- > security/keys/trusted-keys/trusted_tee.c | 24 ++++++++++-------------- > 1 file changed, 10 insertions(+), 14 deletions(-) > > diff --git a/security/keys/trusted-keys/trusted_tee.c b/security/keys/trusted-keys/trusted_tee.c > index 2ce66c199e1d..45f96f6ed673 100644 > --- a/security/keys/trusted-keys/trusted_tee.c > +++ b/security/keys/trusted-keys/trusted_tee.c > @@ -65,7 +65,7 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob) > int ret; > struct tee_ioctl_invoke_arg inv_arg; > struct tee_param param[4]; > - struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL; > + struct tee_shm *reg_shm_in, *reg_shm_out; I don't have this file (trusted_tee.c) in my tree and there's no lore link here to previous what threads this depends on. Based on the context I can't verify that reg_shm_in will always be initialized before you get to the free_shm_in label. > > memset(&inv_arg, 0, sizeof(inv_arg)); > memset(¶m, 0, sizeof(param)); > @@ -84,7 +84,7 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob) > if (IS_ERR(reg_shm_out)) { > dev_err(pvt_data.dev, "blob shm register failed\n"); > ret = PTR_ERR(reg_shm_out); > - goto out; > + goto free_shm_in; > } > > inv_arg.func = TA_CMD_SEAL; > @@ -109,11 +109,9 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob) > p->blob_len = param[1].u.memref.size; > } > > -out: > - if (reg_shm_out) > - tee_shm_free(reg_shm_out); > - if (reg_shm_in) > - tee_shm_free(reg_shm_in); > + tee_shm_free(reg_shm_out); > +free_shm_in: > + tee_shm_free(reg_shm_in); > > return ret; > } > @@ -126,7 +124,7 @@ static int trusted_tee_unseal(struct trusted_key_payload *p, char *datablob) > int ret; > struct tee_ioctl_invoke_arg inv_arg; > struct tee_param param[4]; > - struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL; > + struct tee_shm *reg_shm_in, *reg_shm_out; > > memset(&inv_arg, 0, sizeof(inv_arg)); > memset(¶m, 0, sizeof(param)); > @@ -145,7 +143,7 @@ static int trusted_tee_unseal(struct trusted_key_payload *p, char *datablob) > if (IS_ERR(reg_shm_out)) { > dev_err(pvt_data.dev, "key shm register failed\n"); > ret = PTR_ERR(reg_shm_out); > - goto out; > + goto free_shm_in; > } > > inv_arg.func = TA_CMD_UNSEAL; > @@ -170,11 +168,9 @@ static int trusted_tee_unseal(struct trusted_key_payload *p, char *datablob) > p->key_len = param[1].u.memref.size; > } > > -out: > - if (reg_shm_out) > - tee_shm_free(reg_shm_out); > - if (reg_shm_in) > - tee_shm_free(reg_shm_in); > + tee_shm_free(reg_shm_out); > +free_shm_in: > + tee_shm_free(reg_shm_in); > > return ret; > } > -- > 2.30.2