Re: [PATCH v4 22/24] powerpc/pseries: Implement secvars for dynamic secure boot

["Nicholas Piggin" <npiggin@gmail.com>]

On Tue Jan 31, 2023 at 12:54 PM AEST, Andrew Donnellan wrote:
> On Tue, 2023-01-24 at 15:17 +1000, Nicholas Piggin wrote:
> > > +static const char * const plpks_var_names[] = {
> > > +       "PK",
> > > +       "KEK",
> > > +       "db",
> > > +       "dbx",
> > > +       "grubdb",
> > > +       "grubdbx",
> > > +       "sbat",
> > > +       "moduledb",
> > > +       "trustedcadb",
> > > +       NULL,
> > > +};
> > 
> > Var and key are used somewhat interchangeably? These are keys, I
> > think?
> > And plpks could have other vars but we're only interested in (at
> > least a
> > subset of) keys here if I understood right.
> > 
> > I guess the terminology is like that throughout secvar so maybe
> > nothing
> > to be done.
>
> The "key" terminology seems to come from OPAL, while on the PLPKS side
> it's a bit of a mess but "var" follows the usage in existing code (the
> spec refers more to "objects").
>
> > 
> > > +
> > > +static int plpks_get_variable(const char *key, u64 key_len, u8
> > > *data,
> > > +                             u64 *data_size)
> > > +{
> > > +       struct plpks_var var = {0};
> > > +       int rc = 0;
> > > +
> > > +       var.name = kcalloc(key_len - 1, sizeof(wchar_t),
> > > GFP_KERNEL);
> > > +       if (!var.name)
> > > +               return -ENOMEM;
> > > +       rc = utf8s_to_utf16s(key, key_len - 1, UTF16_LITTLE_ENDIAN,
> > > (wchar_t *)var.name,
> > > +                            key_len - 1);
> > > +       if (rc < 0)
> > > +               goto err;
> > 
> > Okay I can't work out why it's key_len - 1 rather than key_len.
>
> The existing code in secvar-sysfs.c calls secvar_ops->get() with
> key_len = strlen(name) + 1, to include the null byte, which is what
> OPAL expects. For PLPKS, the variable name explicitly does not include
> a trailing null byte.
>
> I'll add a comment indicating as such, perhaps at some later point it
> can be reworked.
>
> > 
> > > +       var.namelen = rc * 2;
> > > +
> > > +       var.os = PLPKS_VAR_LINUX;
> > > +       if (data) {
> > > +               var.data = data;
> > > +               var.datalen = *data_size;
> > > +       }
> > > +       rc = plpks_read_os_var(&var);
> > > +
> > > +       if (rc)
> > > +               goto err;
> > > +
> > > +       *data_size = var.datalen;
> > > +
> > > +err:
> > > +       kfree(var.name);
> > > +       if (rc && rc != -ENOENT) {
> > > +               pr_err("Failed to read variable '%s': %d\n", key,
> > > rc);
> > > +               // Return -EIO since userspace probably doesn't
> > > care about the
> > > +               // specific error
> > > +               rc = -EIO;
> > > +       }
> > > +       return rc;
> > > +}
> > > +
> > > +static int plpks_set_variable(const char *key, u64 key_len, u8
> > > *data,
> > > +                             u64 data_size)
> > > +{
> > > +       struct plpks_var var = {0};
> > > +       int rc = 0;
> > > +       u64 flags;
> > > +
> > > +       // Secure variables need to be prefixed with 8 bytes of
> > > flags.
> > > +       // We only want to perform the write if we have at least
> > > one byte of data.
> > > +       if (data_size <= sizeof(flags))
> > 
> > So it's unstructured 8 byte of flags, not a u64 integer? Why not u8
> > flags[8] then?
>
> No, it's a u64 and it's passed in the hcall as a single u64.

In host endian? This is done so userspace can acces it with the existing
secvar API, right? I suppose that's okay...

> > > +static ssize_t plpks_secvar_format(char *buf, size_t bufsize)
> > > +{
> > > +       struct plpks_var var = {0};
> > > +       ssize_t ret;
> > > +
> > > +       var.component = NULL;
> > > +       // Only the signed variables have null bytes in their
> > > names, this one doesn't
> > > +       var.name = "SB_VERSION";
> > > +       var.namelen = 10;
> > 
> > Could you make that strlen(var.name) for the benefit of those of us
> > with
> > missing fingers?
>
> Will do.
>
> > 
> > > +       var.datalen = 1;
> > > +       var.data = kzalloc(1, GFP_KERNEL);
> > 
> > This could just point to a u8 on stack I think?
>
> Until we get VMAP_STACK and we'll have to switch back.

AFAIKS plpks_read_var does not require linear map, so should not be
required. IMO that's the right way to go for an external API, so
actually plpks_write_var is the outlier there and should be changed
to follow read and remove in not requiring special pointers.

Thanks,
Nick