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 phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 56CA1C433EF for ; Sat, 25 Dec 2021 19:35:23 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id AD0B4830C4; Sat, 25 Dec 2021 20:35:20 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="p9mhsjvc"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 2D4EB834CB; Sat, 25 Dec 2021 20:35:19 +0100 (CET) Received: from mail-ed1-x52c.google.com (mail-ed1-x52c.google.com [IPv6:2a00:1450:4864:20::52c]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 4555983041 for ; Sat, 25 Dec 2021 20:35:15 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ilias.apalodimas@linaro.org Received: by mail-ed1-x52c.google.com with SMTP id j6so45297380edw.12 for ; Sat, 25 Dec 2021 11:35:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=7rQG7QTdfaEhlr0V6YD+DnEHtXGPpD1314mJ0tx5+sA=; b=p9mhsjvcnPTcSYt7pyq+ISXYFFiLKn4czMaiFm3GOx3Sj/CDTeGQOx8dD75D9MJdcO 9HgzM2X8gSlw8tCZS7Wx07FyyLqzJrPGvigUsiFEstrSRXFpB4OYcTpcpKxi20+5XsB4 /44WZ/6V11pbrcAvBnJUTj5UyOLo9rE7N5wmJelBm9mR8ewd1yS4I/uu5tU1BFlnjvPj qhfBtw8PkiHwVdC1JZN1Zazwki8vGbOFCYgfIA+xQAHSw4Wxa2sivaWG53xNJxrr5+12 BXp45gKS0AyUBbcFCiEYywUHR1pdvJ2A9B5g99RyfEpVwv3lJ49Hy5v2HUmoh3BDNp8y lYUg== 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:content-transfer-encoding :in-reply-to; bh=7rQG7QTdfaEhlr0V6YD+DnEHtXGPpD1314mJ0tx5+sA=; b=S8ke1upFM5KtfGS20ojLDw+3r+fcrmqvIYcrPeNPTTdTdVsdbFYYms9F8CbGJajgOC 4I4qgp1bKVMDjMvF11XThNsSiWgjgjNA53TfQHBOu95dQPNKF8+ynANqIaB3G0lFQ/MH e5FNhlsijVXz6BcJW7ts53PQgOVNDfsHRRV+bK76h55XbnYVIY0tYPPicgL+U5J2Ve9g uYklYjVVsEi1rogEVgdJiy0CCIw9g3xd2Uj3Q+98Kdxoloka9N4mewD78ZDY3nyad7k9 rlNKKSXRkjzI51a2K7KTFIijtF05cS85dPITp0zKYueo6ovWQeknvkEssmrkKa8rwefZ yLkA== X-Gm-Message-State: AOAM533n/8rxsVYZbzeZuMBA+EkratdgIwrP0BPhrQ8uIc+RZcDFu040 cREMq2W3lHauISdkT8heTkveRQ== X-Google-Smtp-Source: ABdhPJyPMHpGpC9mfuARCs+Jft2MFG38k+YRYRUJoYXevNQUfaPlo+ctVar4oUvdqbgJlSI1eMORlA== X-Received: by 2002:a05:6402:1e93:: with SMTP id f19mr10540953edf.60.1640460914765; Sat, 25 Dec 2021 11:35:14 -0800 (PST) Received: from hades (athedsl-4461669.home.otenet.gr. [94.71.4.85]) by smtp.gmail.com with ESMTPSA id c8sm4262572edu.60.2021.12.25.11.35.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 25 Dec 2021 11:35:14 -0800 (PST) Date: Sat, 25 Dec 2021 21:35:11 +0200 From: Ilias Apalodimas To: Heinrich Schuchardt Cc: Ilias Apalodimas , Ying-Chun Liu , Jens Wiklander , Alexander Graf , U-Boot Mailing List Subject: Re: [PATCH v2] efi_loader: Don't limit the StMM buffer size explicitly Message-ID: References: <20211224080841.98906-1-ilias.apalodimas@linaro.org> <93e2906d-33a5-34b4-38f3-aad27a4aef36@gmx.de> <86CEA110-CBA3-4661-8742-3C4838D3660C@gmx.de> <93e7b044-163f-4828-abd9-a9631ecc2ae8@gmx.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <93e7b044-163f-4828-abd9-a9631ecc2ae8@gmx.de> X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.38 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean On Sat, Dec 25, 2021 at 05:13:23PM +0100, Heinrich Schuchardt wrote: > On 12/25/21 16:04, Ilias Apalodimas wrote: > > > > > > On Sat, 25 Dec 2021, 16:28 Heinrich Schuchardt, > > wrote: > > > > > > > > Am 25. Dezember 2021 12:16:29 MEZ schrieb Ilias Apalodimas > > >: > > >> > > > >[...] > > >> >    rc = tee_invoke_func(conn.tee, &arg, 2, param); > > >> >    tee_shm_free(shm); > > >> > +  /* > > >> > +   * Although the max payload is configurable on StMM, we > > only share > > >> > +   * four pages from OP-TEE for the non-secure buffer used to > > communicate > > >> > +   * with StMM. OP-TEE will reject anything bigger than that > > and will > > >> > +   * return.  So le'ts at least warn users > > >> > +   */ > > The comment mentioning four pages does not make too much sense to me as > both OP-TEE as well as U-Boot can be configured to other sizes. > > > >> >    tee_close_session(conn.tee, conn.session); > > >> > -  if (rc || arg.ret != TEE_SUCCESS) > > >> > +  if (rc || arg.ret != TEE_SUCCESS) { > > >> > > >> tee_close_session(): Will arg.ret be valid if rc != 0? > > > > > >Depends when tee_invoke_func() failed.  But why do we care? > > > > Should we write a message if rc !=0 && arg.ret == TEE_ERROR_EXCESS_DATA? > > > > > > I don't think its needed. OPTEE will set that only if RC == 0 > > > > Cheers > > Ilias > > > > > > >The connection needs to close regardless and we then have to > > reason with > > >the error. > > > > > >Regards > > >/Ilias > > So how about: > > @@ -114,7 +113,11 @@ static efi_status_t optee_mm_communicate(void > *comm_buf, ulong dsize) > rc = tee_invoke_func(conn.tee, &arg, 2, param); > tee_shm_free(shm); > tee_close_session(conn.tee, conn.session); > - if (rc || arg.ret != TEE_SUCCESS) > + if (rc) > + return EFI_DEVICE_ERROR; > + if (arg.ret == TEE_ERROR_EXCESS_DATA) > + log_err("Variable payload too large\n"); > + if (arg.ret != TEE_SUCCESS) > return EFI_DEVICE_ERROR; We just move the error reporting out of the inner if. i.e if (arg.ret == TEE_ERROR_EXCESS_DATA) log_err("Variable payload too large\n"); if (rc || arg.ret != TEE_SUCCESS) return EFI_DEVICE_ERROR; Which looks better to me Regards /Ilias > > Best regards > > Heinrich > > > >> > > >> > +          if (arg.ret == TEE_ERROR_EXCESS_DATA) > > >> > +                  log_err("Variable payload too large\n"); > > >> >            return EFI_DEVICE_ERROR; > > >> > +  } > > >> > > > >> >    switch (param[1].u.value.a) { > > >> >    case ARM_SVC_SPM_RET_SUCCESS: > > >> > @@ -255,15 +263,6 @@ efi_status_t EFIAPI > > get_max_payload(efi_uintn_t *size) > > >> >            goto out; > > >> >    } > > >> >    *size = var_payload->size; > > >> > -  /* > > >> > -   * Although the max payload is configurable on StMM, we > > only share a > > >> > -   * single page from OP-TEE for the non-secure buffer used > > to communicate > > >> > -   * with StMM. Since OP-TEE will reject to map anything > > bigger than that, > > >> > -   * make sure we are in bounds. > > >> > -   */ > > >> > -  if (*size > OPTEE_PAGE_SIZE) > > >> > -          *size = OPTEE_PAGE_SIZE - MM_COMMUNICATE_HEADER_SIZE  - > > >> > -                  MM_VARIABLE_COMMUNICATE_SIZE; > > >> >    /* > > >> >     * There seems to be a bug in EDK2 miscalculating the > > boundaries and > > >> >     * size checks, so deduct 2 more bytes to fulfill this > > requirement. Fix > > >> > > >