Received-SPF: pass (google.com: domain of linux-kernel+bounces-68616-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 16 Feb 2024 14:07:51 +0100
From: Robert Richter <rrichter@amd.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Alison Schofield <alison.schofield@intel.com>,
	Vishal Verma <vishal.l.verma@intel.com>,
	Ira Weiny <ira.weiny@intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Dave Jiang <dave.jiang@intel.com>,
	Davidlohr Bueso <dave@stgolabs.net>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org,
	Len Brown <lenb@kernel.org>, linux-acpi@vger.kernel.org
Subject: Re: [PATCH v3 3/3] lib/firmware_table: Provide buffer length
 argument to cdat_table_parse()
Message-ID: <Zc9eJ4-WZZo5-a8O@rric.localdomain>
References: <20240209192647.163042-1-rrichter@amd.com>
 <20240209192647.163042-4-rrichter@amd.com>
 <20240214173927.00002d20@Huawei.com>
 <20240214174444.00006c9f@Huawei.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240214174444.00006c9f@Huawei.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?WNDbrFwWq99xNnQqQESyhCYtEwJBprzfEs8+eG1OzV4QZrf9WBUGfIOjJmja?=
 =?us-ascii?Q?R9WbTpfGI3R73KDqIJVODdzOEjm4ETIhG72yFCZmVQ6nYMT+oJVVRyAYDvgM?=
 =?us-ascii?Q?6NLNtwSeI6eBAWhEGm68n+cstRnM2y1prhRrSn4cg79k6naJN25XqPDleJpI?=
 =?us-ascii?Q?ngJaIgKqUgl2A2OE8HfOsWcnCynmOdBGB3J9VvJ3JgbdbUnlwBqNunAFfehY?=
 =?us-ascii?Q?xfCqblv+AjWPLjXXM4XDuIdxOj6i7kjSt2YiF678rNBiFCAHe+4nByMoWVio?=
 =?us-ascii?Q?wCzKl++XP5+nM4vyT4urzzArm3wzmO/u0z2Brx0s9u0UZMc9kaecGq5vN8Hj?=
 =?us-ascii?Q?H0LEEl/3xXCCt/+MtM07T8G6zQ4a6yN7vxBZkOuMQs8L1lO7XtvBnqNOJ8Ir?=
 =?us-ascii?Q?kLsRoLPlS52S1sSan3VWn1KktBdKksz62LO4QJ+K0Rcm6QOz9E8igthTjWD7?=
 =?us-ascii?Q?EwCXA1pBYus+I+ag2Svq5F7lVAm3t2ME80pc8fjvFSy1BDKz1e0D2tgztyDc?=
 =?us-ascii?Q?gN/8v4/z4CO0PMb6rNcC0Eocz5ymi7fXfba51ClbUTnNdoODPQYMs+eMqQFC?=
 =?us-ascii?Q?mbI6UR+GaAQTbsr7AvNhLV+7Z+7N8jIKG/xVcOOizdQ/rNn/qy21Bu1Q25kA?=
 =?us-ascii?Q?5tRdDUGHKrB+xkp0oQ821M2CsyallLJ4l1zglyoKuPbzw+P3WKvdcZ1xe7DO?=
 =?us-ascii?Q?UbCXSgXGXDLdp2S1Sasz/8gDvoc2WrFda0oOWEQOf3GyyMkOacTa69yTyc7l?=
 =?us-ascii?Q?qQU7lNNazmFBpMDVX/DyLWnnaUt8jHc3jLCHPJViilsl3JKnJZiLse9fYkkU?=
 =?us-ascii?Q?CUOOLIFlVdiNz1SJVnHtQI+xbxnloZx6zo7wJGQvSDlXlRARB/nuKpNb2t8L?=
 =?us-ascii?Q?mvtNgc5QYbgENLkgBDxgKFWO2ZEr3KNj6r/AP/fUtWPBmkQSut6V5CWK2iPv?=
 =?us-ascii?Q?/m4NFvTf9qAKU5gcQ8DxkBRB24bRGiVl5TB3EadmVS3mkL4BMiAO7yEPWSgS?=
 =?us-ascii?Q?9LwBZZC4gVypVOYxk1C6vEaO+1uH1I1STy4YcO7Djh2BA3hxR2HYv/TFvSaE?=
 =?us-ascii?Q?7A4E8f+sZdY8WKYsO11TxcCiAFhYDyxSmr73t+uJ7urOGtiNo9Z7EByjyNmj?=
 =?us-ascii?Q?ACwiUY1Mnzy5dtgm4MBm1eUeE4M+uhjqcxiktCKoIWQXvIpX1MIaCWp+a1xS?=
 =?us-ascii?Q?QT8Vwx1piUMDvdkNqMRKdI8OLdjD3CYIVL1j5UYpxPeBTlKK+Ou/eTE6ZkJo?=
 =?us-ascii?Q?tjmbINiHUxRhYfepCS1ITxnjgvoGmVOIvIIveakuiDj8Dkbj6ekUXaRaS4ng?=
 =?us-ascii?Q?0Ntt7gozwnmos1Bfuyn756uTwoKVKS2P1Wacjj9T6CV226vW1lKRjQcHhW35?=
 =?us-ascii?Q?uB76d3v7XL8vGLreLjUH9h+pazyk+69/Q5sw8liYUtPI6bZCrxhk82ANKXu5?=
 =?us-ascii?Q?l1tTyCEdXM+XplUq6fYrI+WopNgBk0JpOOzJOyDNYj0rgjdeTwjuL82wiyM5?=
 =?us-ascii?Q?ZYc64Ry//8vZFRc7BBh0lBZihdlYBS9N7e5Shrz3D3uOlNKnzYW3un8kBvfu?=
 =?us-ascii?Q?FQooXFpdRLSNdk/inpJiADpWB63FwMWX21d6GWFc?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 97155de4-5a2e-4a2f-85de-08dc2ef04ba1
X-MS-Exchange-CrossTenant-AuthSource: CYYPR12MB8750.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Feb 2024 13:07:57.7157
 (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: PX3ife/oCyPDsU0c/2oXNO0lnqkW8gNEi7ggUwnhr296qP3QPnpnPYM3+jbbPWiArTpTeMhAzcZObF//XHcmVQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR12MB6344

On 14.02.24 17:44:44, Jonathan Cameron wrote:
> On Wed, 14 Feb 2024 17:39:27 +0000
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> > On Fri, 9 Feb 2024 20:26:47 +0100
> > Robert Richter <rrichter@amd.com> wrote:
> > 
> > > There exists card implementations with a CDAT table using a fix  
> > There exist ... fixed size buffer,
> > > buffer, but with entries filled in that do not fill the whole table
> > > length size. Then, the last entry in the CDAT table may not mark the
> > > end of the CDAT table buffer specified by the length field in the CDAT
> > > header. It can be shorter with trailing unused (zero'ed) data. The
> > > actual table length is determined while reading all CDAT entries of
> > > the table with DOE.
> > > 
> > > If the table is greater than expected (containing zero'ed trailing
> > > data), the CDAT parser fails with:
> > > 
> > >  [   48.691717] Malformed DSMAS table length: (24:0)
> > >  [   48.702084] [CDAT:0x00] Invalid zero length
> > >  [   48.711460] cxl_port endpoint1: Failed to parse CDAT: -22
> > > 
> > > In addition, a check of the table buffer length is missing to prevent
> > > an out-of-bound access then parsing the CDAT table.
> > > 
> > > Hardening code against device returning borked table. Fix that by
> > > providing an optional buffer length argument to
> > > acpi_parse_entries_array() that can be used by cdat_table_parse() to
> > > propagate the buffer size down to its users to check the buffer
> > > length. This also prevents a possible out-of-bound access mentioned.
> > > 
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > Cc: Len Brown <lenb@kernel.org>
> > > Signed-off-by: Robert Richter <rrichter@amd.com>
> > > Reviewed-by: Dave Jiang <dave.jiang@intel.com>  
> > 
> > I think we should scream a bit about this if we see it
> > as I'm unconvinced the spec allows for an implementation like this.
> > 
> > If the spec is unclear, lets seek a clarification.
> > 
> > I'm fine with this as a defensive measure, I just don't want
> > device vendors to keep doing it! 
> > 
> Scrub that - I got around to checking the CDAT spec. It can
> change length whilst we are reading it due to DSEMTS entry
> counts being allowed to change.
> https://uefi.org/sites/default/files/resources/Coherent%20Device%20Attribute%20Table_1.03.pdf
> (it's in the description of the Sequence field)
> 
> Sure we'll notice that the checksum fails and the sequence number
> has updated but that doesn't help us if we went out of bounds
> before we knew that.
> 
> Definitely good to check this as I think we can hit it even
> if we don't have a potentially buggy device.
> I'd still like to moan if we get inconsistent sizes and it
> isn't a race though. Can we find a clean way to detect this
> at a point where we know we have a valid complete table?

I will add a warning about a length mismatch.

Thanks,

-Robert