Re: [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.g...@gmx.de
<mailto:xypron.g...@gmx.de>> wrote:



    Am 25. Dezember 2021 12:16:29 MEZ schrieb Ilias Apalodimas
    <ilias.apalodi...@linaro.org <mailto:ilias.apalodi...@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
     >>