[PATCH v2] efi_loader: Don't limit the StMM buffer size explicitly

[Heinrich Schuchardt]

On 12/25/21 16:04, Ilias Apalodimas wrote:
>
>
> On Sat, 25 Dec 2021, 16:28 Heinrich Schuchardt, <xypron.glpk at gmx.de
> <mailto:xypron.glpk at gmx.de>> wrote:
>
>
>
>     Am 25. Dezember 2021 12:16:29 MEZ schrieb Ilias Apalodimas
>     <ilias.apalodimas at linaro.org <mailto:ilias.apalodimas at linaro.org>>:
>      >> >
>      >[...]
>      >> >    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;

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
>      >>
>