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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5710fa2a1763fe82ca6ae95a1d3b4cc80be410f7.camel@linux.ibm.com>
Date:   Tue, 10 Jan 2023 14:59:26 +1100
From:   Andrew Donnellan <ajd@...ux.ibm.com>
To:     Michael Ellerman <mpe@...erman.id.au>,
        Russell Currey <ruscur@...sell.cc>,
        linuxppc-dev@...ts.ozlabs.org
Cc:     gregkh@...uxfoundation.org, gcwilson@...ux.ibm.com,
        linux-kernel@...r.kernel.org, nayna@...ux.ibm.com,
        zohar@...ux.ibm.com
Subject: Re: [PATCH v2 7/7] powerpc/pseries: Implement secvars for dynamic
 secure boot

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@...ux.ibm.com   IBM Australia Limited