diff mbox series

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

Message ID 20230130083843.802106-1-Ilia.Gavrilov@infotecs.ru
State New
Headers
Series iommu/amd: @Add a length limitation for the ivrs_acpihid command-line parameter |

Commit Message

Gavrilov Ilia Jan. 30, 2023, 8:38 a.m. UTC 
  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>
---
 drivers/iommu/amd/init.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Comments

Kim Phillips Feb. 2, 2023, 12:44 a.m. UTC | #1 
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
Gavrilov Ilia Feb. 2, 2023, 8:05 a.m. UTC | #2 
On 2/2/23 03:44, Kim Phillips wrote:
> Not sure what that '@' is doing in the subject line...
> 

Sorry, this is my typo.
I'll fix it in V2.

> 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?
> 

I'll add it to V2.
>> ---
>>   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"
> 

I tried to use __stringify, but I didn't find a brief way to do it 
correctly for the expression (ACPIHID_UID_LAN + ACPIHID_HID_LAN). The 
preprocessor does not evaluates a constant, but simply substitutes (256+9).

> but the latter might make the already long sscanf line lengths longer...
>  > Either way:
> 
> Reviewed-by: Kim Phillips <kim.phillips@amd.com>
> 
> Kim

Thank you for review.
diff mbox series

Patch

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;