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