Re: [PATCH v2 7/7] powerpc/pseries: Implement secvars for dynamic secure boot

[Andrew Donnellan <ajd@linux.ibm.com>]

On Fri, 2023-01-06 at 21:49 +1100, Michael Ellerman wrote:
> 
> > +static int plpks_get_variable(const char *key, uint64_t key_len,
> > +                             u8 *data, uint64_t *data_size)
> > +{
> > +       struct plpks_var var = {0};
> > +       u16 ucs2_namelen;
> > +       u8 *ucs2_name;
> > +       int rc = 0;
> > +
> > +       ucs2_namelen = get_ucs2name(key, &ucs2_name);
> > +       if (!ucs2_namelen)
> > +               return -ENOMEM;
> > +
> > +       var.name = ucs2_name;
> > +       var.namelen = ucs2_namelen;
> > +       var.os = PLPKS_VAR_LINUX;
> > +       rc = plpks_read_os_var(&var);
> > +
> > +       if (rc)
> > +               goto err;
> > +
> > +       *data_size = var.datalen + sizeof(var.policy);
> > +
> > +       // We can be called with data = NULL to just get the object
> > size.
> > +       if (data) {
> > +               memcpy(data, &var.policy, sizeof(var.policy));
> > +               memcpy(data + sizeof(var.policy), var.data,
> > var.datalen);
> > +       }
> 
> There's a lot of allocation and copying going on. The secvar-sysfs.c
> data_read() has kzalloc'ed data, but only after already doing the
> hcall
> to get the size. Then plpks_read_os_var() does an allocation for the
> hcall and then another allocation of the exact data size. Then
> data_read()
> does another copy into the sysfs supplied buffer.
> 
> So to read a single variable we do the hcall twice, and allocate/copy
> the content of the variable 4 times?
> 
>  - Hypervisor into "output" in plpks_read_var().
>  - "output" into "var->data" in plpks_read_var().
>  - "var.data" into "data" in plpks_get_variable().
>  - "data" into "buf" in data_read().
> 
> As long as maxobjsize is < PAGE_SIZE I think we could do the hcall
> directly into "buf". Maybe we want to avoid writing into "buf"
> directly
> in case the hcall fails or something, but the other 3 copies seem
> unnecessary.

In the general case, I don't like passing buffer pointers straight from
parameters into hcalls, since the address has to be in the linear map,
and that's a detail I'd rather hide from callers. But otherwise, yes I
think we can probably shift to having the caller allocate the buffers.

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited