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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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.glpk@gmx.de
> > <mailto:xypron.glpk@gmx.de>> wrote:
> > 
> > 
> > 
> >     Am 25. Dezember 2021 12:16:29 MEZ schrieb Ilias Apalodimas
> >     <ilias.apalodimas@linaro.org <mailto:ilias.apalodimas@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
> >      >>
> > 
>