linux-kernel - Re: [PATCH v5 23/25] 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]

Date:   Wed, 01 Feb 2023 17:32:03 +1100
From:   Andrew Donnellan <ajd@...ux.ibm.com>
To:     Stefan Berger <stefanb@...ux.ibm.com>,
        linuxppc-dev@...ts.ozlabs.org, linux-integrity@...r.kernel.org
Cc:     ruscur@...sell.cc, bgray@...ux.ibm.com, nayna@...ux.ibm.com,
        gcwilson@...ux.ibm.com, gjoyce@...ux.ibm.com, brking@...ux.ibm.com,
        sudhakar@...ux.ibm.com, erichte@...ux.ibm.com,
        gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        zohar@...ux.ibm.com, joel@....id.au, npiggin@...il.com
Subject: Re: [PATCH v5 23/25] powerpc/pseries: Implement secvars for dynamic
 secure boot

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