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