Message-ID: <39506e9f-9bbb-fcf6-b488-542fd3657eae@amd.com>
Date:   Wed, 1 Feb 2023 18:44:28 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH] iommu/amd: @Add a length limitation for the ivrs_acpihid
 command-line parameter
To:     Gavrilov Ilia <Ilia.Gavrilov@infotecs.ru>,
        Joerg Roedel <joro@8bytes.org>
Cc:     Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
        Will Deacon <will@kernel.org>,
        Robin Murphy <robin.murphy@arm.com>,
        Wan Zongshun <Vincent.Wan@amd.com>,
        "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "lvc-project@linuxtesting.org" <lvc-project@linuxtesting.org>
References: <20230130083843.802106-1-Ilia.Gavrilov@infotecs.ru>
Content-Language: en-US
From:   Kim Phillips <kim.phillips@amd.com>
Organization: AMD
In-Reply-To: <20230130083843.802106-1-Ilia.Gavrilov@infotecs.ru>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dU1SWFlFNjk2czMwMFEzd2dROE9pWGk2elNoTnpCNDc3M2ZIcTRYK2NJcnlu?=
 =?utf-8?B?UlFuUnd0WnJ0SisxQXp0OFVjRkl2UVR1Y0ZyL3JaUE1TSTFBaTViaFJ5RFB3?=
 =?utf-8?B?ZnhMS1RpWDdKcjB0eXY0d2JSSk9RWHZRNiszV00zaHJQeml2b3I3UVgvdjJr?=
 =?utf-8?B?WDE0SkpvWVlOUTI3dlJuQlZZbnMwQUloSmo4MUhyNE5mWUNMRWpPQ2o1akM1?=
 =?utf-8?B?NndaNlVYVU1ST0JGSlVxeUk1YmhmRXpHUnIxaEIvaHhRK2ZVaWtPLzJERXNP?=
 =?utf-8?B?bXpIUWRJMGxVYi9VTnhBOWFjeUl1dGFJWmF1T20wcUdGQUI1T3BKODZ4U0N1?=
 =?utf-8?B?ejEwUVVOTWliNEh4RkZ5czRuUW9xaVVicFBuZzl1MjIrczBZRElzQmlqR1VY?=
 =?utf-8?B?ek5vUkFmQjJoRkorSis5blVpNlh6bjl1UzNJNHFDVE02K0ZHMFkxSjVGSGZC?=
 =?utf-8?B?VnY0MU9vWWVmYkdjVHY2WTFsRzhiOXgzZlYwaldwY1ozWCtHWFNPcjkrRVZ3?=
 =?utf-8?B?NUQ2MjR2dEprZTc1TFpaRVZIbXRxNGdxMTF2ZTVJZWlDV05pb2JVaTg1YW9y?=
 =?utf-8?B?dVpIanpBdnQ3ZmFVRWt4U3ovNDV3UUttQkpzZHIwWTJPbUpZZFloZERrQ05H?=
 =?utf-8?B?THVxU0d3YjlKWW1RSFNSSTRLVFZNQzM2U2VnYmRDWkJ0SDBaUFlaRXk2eDdL?=
 =?utf-8?B?S3dhZ2lxdzBNZHpCMGMrVDFINHE0K21TRW5hRVNIL3dKRG9ySzUyMFQ5NTk0?=
 =?utf-8?B?bFhIZ0JmdG56MS8yU0lxcWFCTXB0M2I3bE9GUzFEd3IxRVJGTzFLUW9sM3dX?=
 =?utf-8?B?L0Q3Q3dGME56TkxtWkkrMjdWdUcxcno3R1NMbDJ3dnM3WWhFWmpqR0RMWGh6?=
 =?utf-8?B?amtLbENrVElpNkRCbDFUbndQM0s3Z1F2VXhGT2xzK3JFc3JQRUM3VGxzclpp?=
 =?utf-8?B?STdBZElBWGRCTUluMDRRVEV0MzJnLzAwdk1mcEh1Q2ROSDNycldYcGRJVDRL?=
 =?utf-8?B?bUlxWG5xNnFrZ004Z1l1Y3dxSkFieHZQTUVoS3k0SS85SWhDcFFqdE9vZnpz?=
 =?utf-8?B?aDVwSHhxT1lIUHNQTDFUYVMxamdPeEdtWWFtS3kzQkdRQ0hkNmRGVUhHWlZq?=
 =?utf-8?B?U0psWFFKUkxpcFQ2SmIrcjYzOElPdFBzWkFMSkJ4MTg1bjVGMGovYXFsVFox?=
 =?utf-8?B?SlhBc1l5QzE5L0piTmxPdUI3a2FGVkdxVm1CbFFTczVHSlVoZXRmeURQUlc2?=
 =?utf-8?B?cFg2SkVBRWZ4a0R3SEhQZGtMU1dSNTBua1ZJSGw0ckZ4ZW5RUklDbzRYSE9m?=
 =?utf-8?B?WlFCbWsxQThmeit6VjZUNGE3Q2FBcFgrdk4xWjJKTnRKdTBXMU53UFNadm1C?=
 =?utf-8?B?V09nZkVabDg0TkY2L0RJL0JJVHFyNUlMaVZkd2xJWXoyWkxWVllYb1JoaGpF?=
 =?utf-8?B?SlhmbDVnK0JYNlpDQ3lHb09EcnJzRll3dWZlcGFiTFBhWEh5bnU1dEppRGtw?=
 =?utf-8?B?dVV0cGR2VTE4TFBwL2NmV1VMVHlXU1NHTDMvYkcwbDErTS9JaDJLeWxPK0tq?=
 =?utf-8?B?czRZY0dKcjhRQVBWR1ZqNzVlQ3ZoMU5iRGdHU1djM2o5RWFjMTBoczhOLzRo?=
 =?utf-8?B?RzU5WFFtVnRuUG92dkFya3R1bXh0aDRXM1dnK2ZIVGNUUWladjdpZlpzY29G?=
 =?utf-8?B?dEMvY2U3MDNZeEdEU2o2cUozUGU4ZGllaUxmcDY2VHpIMHlwUlBlZlJ1ZDlE?=
 =?utf-8?B?NVVSR082TmdxYWxZQlZLNHhJL3ZiK2JLWXkyei9iUS94WURuWklvcDBHK2VO?=
 =?utf-8?B?K04wYkVlMEFMdU9NSUd1aGFTQmNHVy9GYWJ3Y3pBZ1FaU3VwZFpocFVMRi9p?=
 =?utf-8?B?MGZUUUlmei9Rd0syaVBSaWJaeFB6L3U3ZStpd2tnUXRrckNLeDNQNmsxYkdN?=
 =?utf-8?B?NmxQcjc3VCs0c2FtT3RRY0JvRytZN214di9kZVFnVSt4RFUvcWpnUzhOcE5o?=
 =?utf-8?B?TythMFBrV3dHNUMxVS9KMnNhUjAwSktTSzFNTUZZRGVHTG9hU0NGK3p0M3M2?=
 =?utf-8?B?SnpmenZLUyt5b0xLakxTNzNsZ3o2K2M2TUsvNWd6WExCWGc2N2FpNTdERmtT?=
 =?utf-8?B?bVF0eFMxd3NsWFEyTHJ5S1B1N2UrVTRJOHA5aUhBa0R3TUwxSTB3aFUyTGlO?=
 =?utf-8?Q?NyYwzY+TKet25xcXrCbcpa/yaX/Phu1mg3u3Rnvm1J2e?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 32414902-764f-4983-9269-08db04b6a5cc
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB6263.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Feb 2023 00:44:31.6683
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 7cTVLez3fwW4c/yNafEH3G6tFyckyk5To4mK3sa8/GDgOnc3FKsME8PSCiRwyEKJqjdJg/TIVTA+iUDsLUT1gg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR12MB6574
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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