Re: [PATCH] platform/x86: hp-bioscfg: fix heap buffer overflow in security buffer

[Josh Snyder]

My apologies! It's been a while since my last kernel contribution, and I'm quite
unfamiliar with this driver. All I can say for certain is that this patch fixed
the error I was getting when writing to my homelab PC's BIOS variables, and the
bug looks like a heap overrun to my untrained eyes.

I'll walk through the code as I understand it. The relevant code in
hp-bioscfg/biosattr-interface.c looks like:

int hp_set_attribute(const char *a_name, const char *a_value)
{
...
        a_name_size = hp_calculate_string_buffer(a_name);
        a_value_size = hp_calculate_string_buffer(a_value);
        security_area_size = hp_calculate_security_buffer(auth_token_choice);
        buffer_size = a_name_size + a_value_size + security_area_size;

        buffer = kmalloc(buffer_size + 1, GFP_KERNEL);
        ...
        ret = hp_populate_security_buffer(start, auth_token_choice);
        if (ret < 0)
                goto out_set_attribute;

        ret = hp_wmi_set_bios_setting(buffer, buffer_size);

As you can see, it calls hp_calculate_security_buffer() and then uses the result
to decide how large `buffer` should be. hp_calculate_security_buffer() serves to
predict how many bytes hp_populate_security_buffer() will later write. If
hp_calculate_security_buffer() under-estimates, then
hp_populate_security_buffer() will over-run the kmalloc'd buffer.
hp_populate_security_buffer() has this logic:

        if (strstarts(authentication, BEAM_PREFIX)) {
               ...
        } else {
                /*
                 * UTF-16 prefix is append to the * authbuf when a BIOS
                 * admin password is configured in BIOS
                 */

                /* append UTF_PREFIX to part and then convert it to unicode */
                strprefix = kasprintf(GFP_KERNEL, "%s%s", UTF_PREFIX,
                                      authentication);
                if (!strprefix)
                        return -ENOMEM;

                auth = hp_ascii_to_utf16_unicode(auth, strprefix);
                kfree(strprefix);

                if (!auth) {
                        ret = -EINVAL;
                        goto out_buffer;
                }
        }

The existing logic in hp_calculate_security_buffer() doesn't match: it has these
two early-return guards:

        if (!authentication)
                return sizeof(u16) * 2; 

        authlen = strlen(authentication);
        if (!authlen)
                return sizeof(u16) * 2;

The second early-return, in particular, means that an empty `authentication`
string will produce a buffer that is 16 bytes too small for the payload that is
ultimately written by hp_populate_security_buffer() and sent to the firmware by
hp_wmi_set_bios_setting().

The net effect is that my homelab's firmware saw an invalid payload and refused
the write with error 0x04. My guess is that this bug will hit anyone who doesn't
have a BIOS admin password set, and the logic had only been tested with HP
BIOSes where the admin password is set.

> >  The NULL check is preserved since hp_populate_security_buffer() would
> >  dereference NULL via strstarts().

> What NULL check this refers to??? For which of the inputs?

Yeah, that sentence could have been worded better, I admit. The existing code in
hp_calculate_security_buffer() has two early-return guards, and that sentence is
attempting to answer the question: "why not remove both of them?". The answer is
"because we don't want to pass a null pointer to strstarts()" immediately
beneath:

        if (!strstarts(authentication, BEAM_PREFIX))
                size += strlen(UTF_PREFIX) * sizeof(u16);

But now that I give the code another look, my mildly-informed opinion is that it
should be hp_set_attribute()'s job to ensure a non-NULL `authentication` value as
a precondition of calling hp_calculate_security_buffer().

If any of this "clicks" for you, I'd be happy to reword the commit message to
incorporate whatever portion makes makes the most sense. I'd also be happy to
just contribute a bug report, and have someone with real understanding of this
driver do the actual patching.

Thanks!
Josh