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

[Gavrilov Ilia]

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@xxxxxxxxxxx>
> 
> 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@xxxxxxx>
> 
> Kim

Thank you for review.