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@xxxxxxxxxxxxx IBM Australia Limited
