Re: [PATCH v2] platform/x86: hp-bioscfg: fix slab-out-of-bounds write in hp_convert_hexstr_to_str

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3555 bytes --]

On Tue, 15 Sep 2026, Muhammad Bilal wrote:

> hp_convert_hexstr_to_str() decodes an ACPI string made up of
> space-separated ASCII hex-byte tokens of the form "0xHH". For
> example, the four-byte input "0x41" decodes to the single
> character 'A', and the nine-byte input "0x09 0x41" decodes to
> "\tA": each token is read in a five-byte step (four characters for
> the token, one for the trailing delimiter), and produces one output
> byte, or two if that byte is '\\', '\r', '\n', or '\t', which are
> written back out as a backslash followed by the matching letter.
> 
> The output buffer is sized with kmalloc(input_len, GFP_KERNEL), the
> raw encoded length of the input, not the decoded length. For
> well-formed input this is generous, since five input bytes never
> decode to more than two output bytes. But input_len comes directly
> from the ACPI string length reported by firmware and isn't
> guaranteed to respect the five-byte encoding, so a short input_len
> can undersize the allocation. With input_len == 1, only one byte is
> allocated, yet decoding still produces at least one output byte
> plus the NUL terminator written unconditionally afterwards, so two
> bytes are needed. That terminator write then lands one byte past
> the end of the allocation.

Thanks, very clear now.

I've applied this to the review-ilpo-next branch now, but please see 
below for one additional thing.

> KASAN caught exactly this during BIOS attribute enumeration on
> boot, triggered by a one-byte encoded input value:
> 
> BUG: KASAN: slab-out-of-bounds in hp_convert_hexstr_to_str+0x6d8/0x710 [hp_bioscfg]
> Write of size 1 at addr ffff8881032e5d81 by task (udev-worker)/520
> The buggy address is located 0 bytes to the right of
> allocated 1-byte region [ffff8881032e5d80, ffff8881032e5d81)
> 
> Size the allocation to the worst-case decoded length instead of the
> raw input length: two output bytes for every five-byte input chunk
> (DIV_ROUND_UP(input_len, 5)), plus one byte for the terminator.
> 
> Fixes: a34fc329b189 ("platform/x86: hp-bioscfg: bioscfg")
> Cc: stable@vger.kernel.org
> Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
> ---
> v2: Expand the commit message to spell out the hex-token input
>     format and decoded output with a worked example, and explain
>     exactly how a short input_len undersizes the allocation, per
>     Ilpo Järvinen's review. No code change from v1.
> 
>  drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> index 22c198680903..42331cf90581 100644
> --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> @@ -442,7 +442,7 @@ int hp_convert_hexstr_to_str(const char *input, u32 input_len, char **str, int *
>  	*len = 0;
>  	*str = NULL;
>  
> -	new_str = kmalloc(input_len, GFP_KERNEL);
> +	new_str = kmalloc(2 * DIV_ROUND_UP(input_len, 5) + 1, GFP_KERNEL);
>  	if (!new_str)
>  		return -ENOMEM;
>  
> 

I think the buffer should be allocated with kzalloc() to ensure there's no 
potential leakage when/if the output buffer is filled only partially 
which seems well possible given your description. So it would warrant 
another patch on top of this change (as I've applied this now).


And your other patches are not forgotten, I'll get to them while 
processing the rest of the patch queue.


-- 
 i.