drivers/platform/x86/hp/hp-bioscfg/spmobj-attributes.c | 3 --- 1 file changed, 3 deletions(-)
hp_calculate_security_buffer() returns sizeof(u16) * 2 (4 bytes) for
empty strings via an early return. However, hp_populate_security_buffer()
always prepends UTF_PREFIX ("<utf-16/>") to non-BEAM authentication
strings, including empty ones, before converting to UTF-16. This results
in 20 bytes being written into a 4-byte region of the heap buffer
allocated in hp_set_attribute().
The 16-byte overrun corrupts adjacent heap memory. In practice, the
firmware's own bounds checking rejects the undersized buffer before
acting on it (returning error 0x04), which masked the overflow.
Fix by removing the early return for empty strings. The calculation at
the end of the function already accounts for UTF_PREFIX correctly when
authlen is zero: sizeof(u16) + 0 + strlen("<utf-16/>") * sizeof(u16)
= 20 bytes, matching what hp_populate_security_buffer() writes. The
NULL check is preserved since hp_populate_security_buffer() would
dereference NULL via strstarts().
Fixes: b2715aa2e1352 ("platform/x86: hp-bioscfg: spmobj-attributes")
Assisted-by: Claude:claude-opus-4-6
Signed-off-by: Josh Snyder <josh@code406.com>
---
drivers/platform/x86/hp/hp-bioscfg/spmobj-attributes.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/platform/x86/hp/hp-bioscfg/spmobj-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/spmobj-attributes.c
index 2b00a14792e92..93e0a077e4240 100644
--- a/drivers/platform/x86/hp/hp-bioscfg/spmobj-attributes.c
+++ b/drivers/platform/x86/hp/hp-bioscfg/spmobj-attributes.c
@@ -47,9 +47,6 @@ size_t hp_calculate_security_buffer(const char *authentication)
return sizeof(u16) * 2;
authlen = strlen(authentication);
- if (!authlen)
- return sizeof(u16) * 2;
-
size = sizeof(u16) + authlen * sizeof(u16);
if (!strstarts(authentication, BEAM_PREFIX))
size += strlen(UTF_PREFIX) * sizeof(u16);
---
base-commit: cc13002a9f984d37906e9476f3e532a8cdd126f5
change-id: 20260402-hp-bioscfg-overflow-d58dbe53ecf5
Best regards,
--
JoshOn Thu, 2 Apr 2026, Josh Snyder wrote:
> hp_calculate_security_buffer() returns sizeof(u16) * 2 (4 bytes) for
> empty strings via an early return. However, hp_populate_security_buffer()
> always prepends UTF_PREFIX ("<utf-16/>") to non-BEAM authentication
> strings, including empty ones, before converting to UTF-16. This results
> in 20 bytes being written into a 4-byte region of the heap buffer
> allocated in hp_set_attribute().
>
> The 16-byte overrun corrupts adjacent heap memory. In practice, the
> firmware's own bounds checking rejects the undersized buffer before
> acting on it (returning error 0x04), which masked the overflow.
>
> Fix by removing the early return for empty strings. The calculation at
> the end of the function already accounts for UTF_PREFIX correctly when
> authlen is zero: sizeof(u16) + 0 + strlen("<utf-16/>") * sizeof(u16)
> = 20 bytes, matching what hp_populate_security_buffer() writes. The
> NULL check is preserved since hp_populate_security_buffer() would
What NULL check this refers to??? For which of the inputs?
> dereference NULL via strstarts().
This entire description is confusing and seems to also contradict with
itself in multiple places.
> Fixes: b2715aa2e1352 ("platform/x86: hp-bioscfg: spmobj-attributes")
> Assisted-by: Claude:claude-opus-4-6
> Signed-off-by: Josh Snyder <josh@code406.com>
> ---
> drivers/platform/x86/hp/hp-bioscfg/spmobj-attributes.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/platform/x86/hp/hp-bioscfg/spmobj-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/spmobj-attributes.c
> index 2b00a14792e92..93e0a077e4240 100644
> --- a/drivers/platform/x86/hp/hp-bioscfg/spmobj-attributes.c
> +++ b/drivers/platform/x86/hp/hp-bioscfg/spmobj-attributes.c
> @@ -47,9 +47,6 @@ size_t hp_calculate_security_buffer(const char *authentication)
> return sizeof(u16) * 2;
>
> authlen = strlen(authentication);
> - if (!authlen)
> - return sizeof(u16) * 2;
> -
> size = sizeof(u16) + authlen * sizeof(u16);
> if (!strstarts(authentication, BEAM_PREFIX))
> size += strlen(UTF_PREFIX) * sizeof(u16);
>
> ---
> base-commit: cc13002a9f984d37906e9476f3e532a8cdd126f5
> change-id: 20260402-hp-bioscfg-overflow-d58dbe53ecf5
>
> Best regards,
> --
> Josh
>
--
i.
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
On Sat, 11 Apr 2026, Josh Snyder wrote:
> My apologies! It's been a while since my last kernel contribution, and I'm quite
> unfamiliar with this driver.
Hi,
We're in the same boat when it comes to understanding this driver. This
driver unfortunately got in half-baked (with outstanding comments from me)
and we now have to clean up the mess.
> 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.
Can you try to rephrase text related to getting the 0x04 error in the
changelog text such that it's clear you actually hit the problem while
trying to do X.
> 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);
Oh, I see it now.
I read earlier reading hp_ascii_to_utf16_unicode() and utf8s_to_utf16s().
When reading utf8s_to_utf16s(), I concluded it is prepared to prevent
buffer overrun (I was expecting that).
However, what I missed is that hp_ascii_to_utf16_unicode() happily gives
input size as maxout to utf8s_to_utf16s() which is IMO also a major
problem here. I think that was the missing piece here (this should
definitely be mentioned in the changelog).
So there are two related problems:
- Miscalculation of the buffer length.
- Passing the input size to maxout which prevents utf8s_to_utf16s() from
catching the buffer overrun.
I suggest adding another patch to fix the second problem so we avoid the
overrun if there would be another problem in the size calculation.
> 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);
No, you wrote something entirely different. What you actually wrote is
that the dereference occurs in hp_populate_security_buffer()?!?
(= big confusion, see below)
> 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().
...The thing is hp_populate_security_buffer() is called with the same
auth_token_choice argument and it will pass the same input to strstarts()
without any checking so it seems buggy anyway if auth_token_choice is
NULL, unless I again miss something else.
Also, auth_token_choice cannot ever be NULL. It comes from two sources:
.auth_token is checked and .current_password cannot ever be NULL. So
AFAICT hp_populate_security_buffer() is never given NULL, to state it
does deref one, too, is confusing.
> 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.
When you stated: "In practice, the firmware's own bounds checking rejects
the undersized buffer" I assumed the buffer overrun did not occur because
as per the literal interpretation buffer given to firmware was
"undersized" and wording perhaps just came from AI using terminology in
confusing manner when it meant utf8s_to_utf16s() had caught the problem
(which was my assumption at the time, ie., that the buffer overrun is
prevented by utf8s_to_utf16s()).
My goal here is to have the commit message written such that everyone,
even those not at all familiar with this driver, could follow what blows
up and where. I've some level of familiarity, but not much, and if I
cannot follow the description, likely those entirely unfamilar can even
less.
I'm asking question to understand the problem. Each question usually
implies there's something missing from / wrong in the changelog. It's
not meant to deter you from pursuing the change.
--
i.
© 2016 - 2026 Red Hat, Inc.