Re: [PATCH v5 23/25] powerpc/pseries: Implement secvars for dynamic secure boot

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

On Tue, 2023-01-31 at 12:11 -0500, Stefan Berger wrote:
> > +static int plpks_get_variable(const char *key, u64 key_len, u8
> > *data,
> > +                             u64 *data_size)
> > +{
> > +       struct plpks_var var = {0};
> > +       int rc = 0;
> > +
> > +       // We subtract 1 from key_len because we don't need to
> > include the
> > +       // null terminator at the end of the string
> > +       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;
> > +       var.namelen = rc * 2;
> 
> Are you sure this is not just supposed to be 'rc'?

namelen is a length in bytes, not characters, while utf8s_to_utf16s()
returns the number of characters. I suppose this could be clearer by
changing 2 to sizeof(wchar_t).

> 
> > +
> > +       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))
> > +               return -EINVAL;
> > +
> > +       // We subtract 1 from key_len because we don't need to
> > include the
> > +       // null terminator at the end of the string
> > +       var.name = kcalloc(key_len - 1, sizeof(wchar_t),
> > GFP_KERNEL);
> here I would think that it should be key_len * 2 - 1 since
> utf8s_to_utf16s presumably makes the string longer

No, wchar_t is u16, so this allocates (key_len - 1)*sizeof(u16) =
(key_len - 1)*2 bytes.

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