Messages in this thread | | | From | Gavrilov Ilia <> | Subject | Re: [PATCH] iommu/amd: @Add a length limitation for the ivrs_acpihid command-line parameter | Date | Thu, 2 Feb 2023 08:05:22 +0000 |
| |
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.
| |