linux-kernel - Re: [PATCH] iommu/amd: @Add a length limitation for the ivrs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 1 Feb 2023 18:44:28 -0600
From:   Kim Phillips <kim.phillips@....com>
To:     Gavrilov Ilia <Ilia.Gavrilov@...otecs.ru>,
        Joerg Roedel <joro@...tes.org>
Cc:     Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
        Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Wan Zongshun <Vincent.Wan@....com>,
        "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "lvc-project@...uxtesting.org" <lvc-project@...uxtesting.org>
Subject: Re: [PATCH] iommu/amd: @Add a length limitation for the ivrs_acpihid
 command-line parameter

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@...otecs.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@...x:%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@....com>

Kim