From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by mx.groups.io with SMTP id smtpd.web09.10896.1660614561284568308 for ; Mon, 15 Aug 2022 18:49:21 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="unable to parse pub key" header.i=@intel.com header.s=intel header.b=nj++k2f1; spf=pass (domain: intel.com, ip: 192.55.52.115, mailfrom: ray.ni@intel.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1660614561; x=1692150561; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=jYU0CvRyuMGzzgIT+oCb0l2fbUISUpQZTLEi0xIl6Uc=; b=nj++k2f1OPHh0wwJLnNunMIStL6f7uqE9ref9M7XKgbgTWXXJBhPwaQo W7OPuJ0nPVwYB9MRSAtdiFSDGDpPk6r6lTSorqc17FFsf+fQvlFuIqt2D csGfMJ/xtFfbrr7/Cy/m+8zzURYqtk0fEez3alhDIf/KYQMdLF6bBbhL0 24b5ddvjFpZQsjKwdqKdwBVpiGlR2weFl97BqjCMZTyub8PcuvUvGdQQ4 rzFnNNmwCNp4Dsue0buOtHRU+Q2+gAtI7vaJxy2bjR7AvT1vxHZf7op9O 1w5/qByN7BeODPQZkCMbfQf+xDh4g5oKdw7jM3UqJR/MyrN2hOGCAXsYa A==; X-IronPort-AV: E=McAfee;i="6400,9594,10440"; a="292096966" X-IronPort-AV: E=Sophos;i="5.93,239,1654585200"; d="scan'208";a="292096966" Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Aug 2022 18:49:20 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.93,239,1654585200"; d="scan'208";a="675031244" Received: from fmsmsx602.amr.corp.intel.com ([10.18.126.82]) by fmsmga004.fm.intel.com with ESMTP; 15 Aug 2022 18:49:20 -0700 Received: from fmsmsx608.amr.corp.intel.com (10.18.126.88) by fmsmsx602.amr.corp.intel.com (10.18.126.82) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.28; Mon, 15 Aug 2022 18:49:19 -0700 Received: from fmsmsx603.amr.corp.intel.com (10.18.126.83) by fmsmsx608.amr.corp.intel.com (10.18.126.88) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.28; Mon, 15 Aug 2022 18:49:19 -0700 Received: from FMSEDG603.ED.cps.intel.com (10.1.192.133) by fmsmsx603.amr.corp.intel.com (10.18.126.83) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.28 via Frontend Transport; Mon, 15 Aug 2022 18:49:19 -0700 Received: from NAM11-DM6-obe.outbound.protection.outlook.com (104.47.57.169) by edgegateway.intel.com (192.55.55.68) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2375.28; Mon, 15 Aug 2022 18:49:19 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=bWqhJoEV41zaCcw0vVRg8d/9x5hl4m4Jl3xh71jifX31a6Xy3AbPUKxWnYQIQOt8sAP8lmlQTYdUkqG6QMM0LBf22AXxyYjsVVQ3ogrilPtgPqXSmM1b83bnoYsQtaMZB70btbrAUKnU6jcGFV8YUq9aehW3+F2sshkQhjf6vh7DuIJfGvy7nC4y50L0cjrwSRtXIfHTLzHDObDboQ5deLYDmFwfUP0UB+ChWFhAqxUkfZgYBkaDmfcHvB45u2AZ2ug5okSxKB8N4/74y+2Yry82JEw4G6FBeDRkfkho7eTr5KcDj9mdaifIj/hRqgHwfkuh+aPHXn6+OmuzMjTMGQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=4NEwMECWy2P8zDNtCtceAPx+DzYlnDIQubuu1cKTYgA=; b=i9yR3cR2j47e2AoAI3T1tfCOUWoHGv8xWFTq0Y8vSbEFVJ4L0HFttoQ2mC/hV69WM0+p2MU92ou1+pyJMBd0HP8Ln7KVgXmDtor3uz+Yfc9HO0fwuGWGKD927Nm5q7LjR5/1DbQNDolQO8Fk3dftHl+msxCFTn0NcBtvYZVrRzB9DHAFGMnC7P/gg5BCkj5ON0tiDIQmNj1sAUSB7PcPIox187RpeEvIjAKpVqo0b6JiAoxeU71+gZnX3FMKPFiRirWzb6E8nDTv5zIR1VmqIYarDBa8j4vFZjJXxg77Ce6CqKNB54tDZQmCGYDMqn8RieAnZHC6h5q8HdSXo9NZIA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none Received: from MWHPR11MB1631.namprd11.prod.outlook.com (2603:10b6:301:10::10) by MN2PR11MB4757.namprd11.prod.outlook.com (2603:10b6:208:26b::19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5525.10; Tue, 16 Aug 2022 01:49:16 +0000 Received: from MWHPR11MB1631.namprd11.prod.outlook.com ([fe80::958b:cc9d:ac3:288a]) by MWHPR11MB1631.namprd11.prod.outlook.com ([fe80::958b:cc9d:ac3:288a%10]) with mapi id 15.20.5504.027; Tue, 16 Aug 2022 01:49:16 +0000 From: "Ni, Ray" To: "devel@edk2.groups.io" , "xiewenyi2@huawei.com" , "Wang, Jian J" , "Gao, Liming" , "Dong, Eric" CC: "songdongkuang@huawei.com" Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1] MdeModulePkg/PiSmmCore:Avoid overflow risk Thread-Topic: [edk2-devel] [PATCH EDK2 v2 1/1] MdeModulePkg/PiSmmCore:Avoid overflow risk Thread-Index: AQHYsJyaJa0c4qJe4UGDsi6L41fVtq2wwNMw Date: Tue, 16 Aug 2022 01:49:15 +0000 Message-ID: References: <20220815114529.665138-1-xiewenyi2@huawei.com> <20220815114529.665138-2-xiewenyi2@huawei.com> In-Reply-To: <20220815114529.665138-2-xiewenyi2@huawei.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=intel.com; x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 191e7b87-05c8-4a6b-f4b7-08da7f2986d4 x-ms-traffictypediagnostic: MN2PR11MB4757:EE_ x-ld-processed: 46c98d88-e344-4ed4-8496-4ed7712e255d,ExtAddr x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: UrTvFtm6FLgxd7fCRrPUGtuQCNgXJSUp62ZfB8DkGi6nm1bLohetKvQ85sOM/iztvyKFrM1ZOluPJ1B+9p4IhQszAO8ZQu+giM7KYa4EMcbx4veO+D6IdkWY9EpF/3ZwBJVPzqdaaty659FXN6yUpu2xxS34PyN7+/eqBN/JaCr/0Lsyhfl7qKjUIIZJmIz23V2NmaosAb4rgw3qiaK8uX7wvGX+qDZvkgW4wluITb7pxuvuiZ8/Gv9608Wa/4JhckAnodZtD/5jyCfr7CD/3/lqw3duP9HaZ5unttDT2bBQMLe72z5uGj4QNSvaPdY/iCcSW1AjymYveq1w1mDDxVbDLlCutEbFnjPOPl60z7Nac4Ad60N6lBv46iSanAg7Exb9dPl4txcjQWyQkzCa2nOyW9eO8XpS/3idBZdxwx8QshEJannC6Om5lJi53Mi1NwRs9ZQgYhoBYDxkyPo7Mhq6iQQ0NmX3BW4voRv4Kx8L3nLsjZr5pOx+px9Mj4QaQh1fLLF7IGKn/O6QTpZSL9kZ6WUWQELTeIgN3miE/2Hq/9UknCLGdbCW4+ydXG6HcPMcWIu0uyyUQOsb0ssiKXidDMeusWz2dB3ATwUnJ48C+iK3gobcRDzp8vg6yMYm/4zSCsjRHgXVDcYJA9YjyZbGFXBycEBDTjyZinZHbR56cZagbeIbcjYDY1Z0c8Pontq5iRfVZVuZ0A7P9BMiO3IqTg7+4x2eoV9TkBYWUdCgKs4R/2JjTQvmykGnv+DaEC+ux8BbdGdnm8o72eMgfg542wY96v2LwB6FsBx77q//hUnZJhabPFiFQNzewv9I x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:MWHPR11MB1631.namprd11.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230016)(6029001)(366004)(346002)(39860400002)(396003)(376002)(136003)(38100700002)(316002)(86362001)(66556008)(64756008)(66446008)(8676002)(66476007)(4326008)(82960400001)(66946007)(122000001)(55016003)(38070700005)(52536014)(8936002)(6506007)(76116006)(5660300002)(83380400001)(478600001)(7696005)(186003)(26005)(6636002)(33656002)(71200400001)(110136005)(41300700001)(2906002)(9686003);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?eaasIk90k1uy+UXv/gKAyXbY5piy4Su/mXGuiFpoC0kE8WKAR7DKqUzc9ujm?= =?us-ascii?Q?cHDrfJufAIXKFK/REf3wnVMBLgeLwnOhkvwE82UJag08XWgzYOmt+GJ5BnJC?= =?us-ascii?Q?k1J19WkzikUfAaHDzARZVoHfDsAguXIxFF5Yl7Co5uA9Gszy9QfV2jcwsBFQ?= =?us-ascii?Q?clzhIEkIsuDFIddPMSJ++o+SI1TqWuy6exnxnSfShRyhpeKZ5cPOZTdfU7ee?= =?us-ascii?Q?m9k2W4Or/znAnYgWNyr6hjVdZiFhzRmokKK38EgoIhwNiSMIq8rVjC/8tmvV?= =?us-ascii?Q?Mo4rpfAkkiHKzAhNFpz4LLlOjFXWRX4YdR+VmPzasBEX4x9YtOj4tuw3v2im?= =?us-ascii?Q?KZKUGN9MjmYFHgcR6qG5UUXoKBSy+o83WBNuPvqZI2AP5vyEwC8DtytXYTIK?= =?us-ascii?Q?Nu7CXwqYQjW0ZYAsHww3UohcFzCkvjhrPfmGjSCxwUBfDQMAkU9Wybxl02QK?= =?us-ascii?Q?izjGdCOVi43v5OjMo3zytNacchW34yG7ZafTBy4uCznQV/As/jR4ufQlw/Rq?= =?us-ascii?Q?2/7U7etNOEv5OOz0mNhdHEr0hwQGFwXS8bk60TDs99OOi4noj1kkvlHPbm6e?= =?us-ascii?Q?8Y913qxHnZpMqE64llIy042N+SebF03IQ6JpmzslH7nKbdZiLi0kV/EF5auZ?= =?us-ascii?Q?uh0MPkQwemMFwwOdknVG3ci3mjB8OPlG9vR5P3V1kO3nGDbl9pmcnhMzzEco?= =?us-ascii?Q?UvaFoApwsb5aGKbshI5aPBDglvHfxSYQqoS4k+asVv04ftaL9Wa/u0Pkezhc?= =?us-ascii?Q?mFwSTcM8OsEMg3a0c3aXcMKLw5SME4c7uopAxnfP2EP8AjfDngfknxMihV19?= =?us-ascii?Q?FxDgysYfJWtdVp28BVDQHRQLnZfvPGuplp9TvyB5r7OxyY+OnR8lil0+XSOx?= =?us-ascii?Q?pG7DDn+lBoEXeJlSv9RCIUsQXgzIk4bNF0dlRHunOsYn1yVh/zgktUUs6osH?= =?us-ascii?Q?HDgWi99NcQiraK0GDVG2eY1Fmtll4ED6bOxUs43lo8dNh65hiOvYt1Do4TPv?= =?us-ascii?Q?IF3no2mohSam1f31jgdYDSFhvK7Nk1SvMKCq8Hx5ETmATb81nadsxSKJx3eA?= =?us-ascii?Q?W2BnzhyUWooDxtVjtZq0T1csNdITLXFXL4Y/XZn6jBs7DKIN+QQCiqCFIJEG?= =?us-ascii?Q?Z6+fhp5633zHYaXigMZPiHRuVna5gsfP9Fcoun1npz+iJ+IXHWGqVuZTfKmW?= =?us-ascii?Q?WFTnpDuOJguOY4UW54V3jR3pl2yeBYURhpsNcB+c1OwnFU4uvh3vQVgYZ+ap?= =?us-ascii?Q?AJjEviiAy6/az+jVFe2+vZe68n+H2/VrgTn7zyohIB7y+O5oQDDIS4HCqtIB?= =?us-ascii?Q?W+a5KWMxSInYFiMY3yl0TdbAsXQa2jI2CkWRdw6vTi5tcSbeKEduEy47zJLL?= =?us-ascii?Q?fzJQrTsJ5abKaYq08YsUlSljDbdsyFyVtbF9FnC94QjUraho3tg3kOSX9Zh0?= =?us-ascii?Q?Mi3rjFSllqVjG8VWtDzPlp/GKdWmKgyZuZIph+0jiQsYSHtDIqGJQ4mUw8WN?= =?us-ascii?Q?iXZYuI1/JiTP9k7etsGXjD12YbeqhhR9Zm5sFEzFwuDNXMLUU5OKSyPqlhN7?= =?us-ascii?Q?cRBHe1ybQK64eR6sMfq/V4SlE6e4yZNXtljIeKC7?= MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: MWHPR11MB1631.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 191e7b87-05c8-4a6b-f4b7-08da7f2986d4 X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Aug 2022 01:49:15.8367 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: OqSPrRV6Lj4NNobMfK1/8b76NskL2PTj+g5S2zAwV4h0u8e6dVBKR69TtzM+VH48Fe+r28RYm7RGNu2ENZzfkw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR11MB4757 Return-Path: ray.ni@intel.com X-OriginatorOrg: intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c > +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c > @@ -621,6 +621,9 @@ InternalIsBufferOverlapped ( > IN UINTN Size2 > ) > { > + if (((UINTN)Buff1 > MAX_UINTN - Size1) || ((UINTN)Buff2 > MAX_UINTN - > Size2)) { > + return TRUE; > + } 1. The change looks good because it avoids integer overflow in below code t= hat adds Size1 to Buff1 and adds Size2 to Buff2. Can you please add comments to explain the logic? >=20 > + if (CommunicateHeader->MessageLength > MAX_UINTN - OFFSET_OF > (EFI_SMM_COMMUNICATE_HEADER, Data)) { > + return EFI_INVALID_PARAMETER; > + } 2. Above check avoids integer overflow in below code that adds CommunicateH= eader->MessageLength to OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data). Can it be moved to inside the below if-clause? > + > if (CommSize =3D=3D NULL) { > TempCommSize =3D OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) > + CommunicateHeader->MessageLength; > } else { 3. I further reviewed the else-clause logic. When CommSize is not NULL, is= that needed to make sure that *CommSize >=3D OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + Communic= ateHeader->MessageLength? Or is the check already in the code somewhere? If we think the check is needed, I agree with the change #2 to have a commo= n logic to check integer overflow.