Re: [PATCH v3 1/2] platform/x86: hp-bioscfg: fix 16-byte heap overflow for empty auth token

On Wed, 19 Aug 2026, Muhammad Bilal wrote:

> hp_calculate_security_buffer() special-cases an empty authentication
> string and returns a fixed 4 bytes (sizeof(u16) * 2). But
> hp_populate_security_buffer() does not special-case that same input:
> for any authentication string that does not start with BEAM_PREFIX,
> including the empty string, it always builds "UTF_PREFIX +
> authentication" and converts the result to UTF-16, writing a 2-byte
> length header plus 2 bytes per character of "<utf-16/>" (9 characters),
> 20 bytes total, regardless of how long "authentication" itself is.
> 
> The caller, hp_set_attribute(), sizes its kmalloc() buffer using
> hp_calculate_security_buffer()'s return value, so for an empty
> authentication token it allocates 4 bytes for the security area but
> hp_populate_security_buffer() then writes 20 bytes into it, causing a
> 16-byte heap buffer overflow.
> 
> The authentication token used here is the current admin/setup
> password, which is an empty string by default until one is
> configured. Any write to a writable BIOS attribute while no admin
> password has been set reaches this path.
> 
> Fix by removing the special-case early return for an empty string in
> hp_calculate_security_buffer(). The generic calculation that follows
> already accounts for the UTF_PREFIX correctly, which naturally yields
> the same 20 bytes that hp_populate_security_buffer() writes for an empty
> string, avoiding duplicate logic for special cases.
> 
> Fixes: b2715aa2e135 ("platform/x86: hp-bioscfg: spmobj-attributes")
> Reported-by: Josh Snyder <[email protected]>
> Closes: https://lore.kernel.org/platform-driver-x86/[email protected]/
> Cc: [email protected]
> Signed-off-by: Muhammad Bilal <[email protected]>
> ---
> Changes in v3:
>   - Remove the special-case return entirely instead of adjusting its
>     formula, avoiding code duplication as suggested by Ilpo Järvinen.
>   - Credit Josh Snyder who previously noted this approach.
> 
> Changes in v2:
>   - None for this patch; resubmitted as part of the v2 series.
> 
> Link: https://lore.kernel.org/r/[email protected] [v1]
> Link: https://lore.kernel.org/r/[email protected] [v2]
> ---
>  drivers/platform/x86/hp/hp-bioscfg/spmobj-attributes.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp/hp-bioscfg/spmobj-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/spmobj-attributes.c
> index 4d94e48c1a4c..136585141e6e 100644
> --- a/drivers/platform/x86/hp/hp-bioscfg/spmobj-attributes.c
> +++ b/drivers/platform/x86/hp/hp-bioscfg/spmobj-attributes.c
> @@ -47,10 +47,6 @@ size_t hp_calculate_security_buffer(const char *authentication)
>  	if (!authentication)
>  		return sizeof(u16) * 2;
>  
> -	authlen = strlen(authentication);
> -	if (!authlen)
> -		return sizeof(u16) * 2;
> -
>  	authlen = strlen(authentication);
>  	size = sizeof(u16) + authlen * sizeof(u16);

authlen is not uninitialized, so only the early return should be dropped.

I've dropped this patch from the review-ilpo-next branch.

-- 
 i.