Re: [PATCH] iommu/amd: @Add a length limitation for the ivrs_acpihid command-line parameter

[Kim Phillips <kim.phillips@amd.com>]

Not sure what that '@' is doing in the subject line...

On 1/30/23 2:38 AM, Gavrilov Ilia wrote:
> The 'acpiid' buffer in the parse_ivrs_acpihid function may overflow,
> because the string specifier in the format string sscanf()
> has no width limitation.
> 
> Found by InfoTeCS on behalf of Linux Verification Center
> (linuxtesting.org) with SVACE.
> 
> Fixes: ca3bf5d47cec ("iommu/amd: Introduces ivrs_acpihid kernel parameter")
> Signed-off-by: Ilia.Gavrilov <Ilia.Gavrilov@infotecs.ru>

cc: stable?

> ---
>   drivers/iommu/amd/init.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 467b194975b3..19a46b9f7357 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -3475,15 +3475,26 @@ static int __init parse_ivrs_hpet(char *str)
>   	return 1;
>   }
>   
> +#define ACPIID_LEN (ACPIHID_UID_LEN + ACPIHID_HID_LEN)
> +
>   static int __init parse_ivrs_acpihid(char *str)
>   {
>   	u32 seg = 0, bus, dev, fn;
>   	char *hid, *uid, *p, *addr;
> -	char acpiid[ACPIHID_UID_LEN + ACPIHID_HID_LEN] = {0};
> +	char acpiid[ACPIID_LEN] = {0};
>   	int i;
>   
>   	addr = strchr(str, '@');
>   	if (!addr) {
> +		addr = strchr(str, '=');
> +		if (!addr)
> +			goto not_found;
> +
> +		++addr;
> +
> +		if (strlen(addr) > ACPIID_LEN)
> +			goto not_found;
> +
>   		if (sscanf(str, "[%x:%x.%x]=%s", &bus, &dev, &fn, acpiid) == 4 ||
>   		    sscanf(str, "[%x:%x:%x.%x]=%s", &seg, &bus, &dev, &fn, acpiid) == 5) {
>   			pr_warn("ivrs_acpihid%s option format deprecated; use ivrs_acpihid=%s@%04x:%02x:%02x.%d instead\n",
> @@ -3496,6 +3507,9 @@ static int __init parse_ivrs_acpihid(char *str)
>   	/* We have the '@', make it the terminator to get just the acpiid */
>   	*addr++ = 0;
>   
> +	if (strlen(str) > ACPIID_LEN + 1)
> +		goto not_found;
> +
>   	if (sscanf(str, "=%s", acpiid) != 1)
>   		goto not_found;
>   

That works, or, this fix might be able to be made more brief if
we could transform all the sscanf's '%s's to:

"%" __stringify(ACPIID_LEN) "s"

but the latter might make the already long sscanf line lengths longer...

Either way:

Reviewed-by: Kim Phillips <kim.phillips@amd.com>

Kim