Date:   Fri, 27 Jan 2023 11:48:59 -0800
From:   Dan Williams <dan.j.williams@intel.com>
To:     Robert Richter <rrichter@amd.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        "Ira Weiny" <ira.weiny@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        Dan Williams <dan.j.williams@intel.com>,
        Dave Jiang <dave.jiang@intel.com>
CC:     Jonathan Cameron <jonathan.cameron@huawei.com>,
        Robert Richter <rrichter@amd.com>, <linux-cxl@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v4] cxl/mbox: Fix Payload Length check for Get Log command
Message-ID: <63d42aaaef012_3a36e52942f@dwillia2-xfh.jf.intel.com.notmuch>
References: <20230119094934.86067-1-rrichter@amd.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230119094934.86067-1-rrichter@amd.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?S+vHVixCmMHwQHCyHmyNTYudYRi5471M7clsIqdpf3eh0+NWc5Gpab7kEHsj?=
 =?us-ascii?Q?l21WW21yLY9NgGszQnDyyF22nd6Qz40xTmvFtwnm8lqqo5PH+yMqsNIgrY/p?=
 =?us-ascii?Q?zDEiVxJ8cAmkYgYTDvYCTvXUlDiYkmWA7dWOL/da5lZkWb9JcZf120pevvc0?=
 =?us-ascii?Q?z74xkNiuzcIJM4HkGAhTq3O/Ud5NCKji0SGpHNetM14zChFSORAJOJxFVkLn?=
 =?us-ascii?Q?ggEy8knv9no/Rjtzk1kR2VQWs73NwiaKnCxL2weJ0a7VlG9OjnD0fZ+LKj9w?=
 =?us-ascii?Q?gRXCOKtK7+UV/hLGth3eKqmEzlyby9zTOQEmz22pkqTrl4CUIZ64znIuRVZA?=
 =?us-ascii?Q?CyAHCeFBaaQMgViJuRYsVwZDxo+NcKKSQ9fRFOHXzLcICXQNPnPUstW6Bp9J?=
 =?us-ascii?Q?uR6KoYS/pGTrMFw19oD65TMr3DWzvaqCEgdH2ltBrcXSJM0g84+JD1bPcm7b?=
 =?us-ascii?Q?crGMUWBuYF8VvlTekI/C5gH1ZeFdZZDX0qVPKu/nijvBz+pOzOHuj2OGyLtt?=
 =?us-ascii?Q?Lj+GTWw9l0oNdwxsYgLy3bkUKY+iOo2fsTQ2tn6GMtYvvkl6QGEhw3VxXKH0?=
 =?us-ascii?Q?0MOW9dUp5y8q+Vo/7/YBpYD75fdNR2lGj4lAj0mEwjZJXiWQUFUAiKqxM1UM?=
 =?us-ascii?Q?VzHLyJ/iZqLw5fpGtDe2yGYQikNMrHdBweKfzz9MTiVY5nGrv96rmRFrAvN+?=
 =?us-ascii?Q?4gcBobcxrw1HbF7mrw4AfSITaJkR60VFiAqyOuTinKUxDorblpYYYTVTZLB0?=
 =?us-ascii?Q?ZVvmBIZYNnZtqmc1cJt0mBX/uLO2UYClori4Zn2pPVhM/kBD0dfBYoR/SZBw?=
 =?us-ascii?Q?Nz7MckX8mPZ0tUnhggO9WKMfnHdUzJDyvjzzOtbHPNlki5vthwsZxGBqnojy?=
 =?us-ascii?Q?EcobmtifwTLUQxUSNjsZBzKrEnnnfiS17k3dLmzggt3t5zxw7eTisbKSSKGq?=
 =?us-ascii?Q?1hQV+2Jfx15zb9lQPLaPQ1jNZghk89lnURmxCfjpDJyEdFafsXv39HWa1k14?=
 =?us-ascii?Q?CUXlQq3ndSGZypQFBcAz7MaXTPpB4wIbFUosZqzGISXxfKwqVz7hCH+8SyKE?=
 =?us-ascii?Q?1TL9wJzUZnxsyLMIt6rkbeynJ+DEOfnev4I6RwvmMP0YSBHc4GK/FVseBrIa?=
 =?us-ascii?Q?886815oWUs8ewV21ROzfvRv64sVTipAlQMjw06kLNPcS2A0B+CpJLILsOlEe?=
 =?us-ascii?Q?lSVlrCel12anZaMEdDIop2WS9Adt8/1zPIqeUXqkhq6k7TBYL0MYeyglB/Uq?=
 =?us-ascii?Q?zbzWOl0ZhsvqXHGRpbM+v47hBapyIjKKlvbNaRq6aO31ektFgUI4FwBAoAhr?=
 =?us-ascii?Q?GrA5rn2DUni3JcDmXbf8ls2ACy39IfdJmxTaxm3mxOfzaRtUHs4jvWg7c9XJ?=
 =?us-ascii?Q?hKll0wnIVoQyWvvHwY1wgXSK6gn89AF5sDixXta5BFaDM6PHg76iDlqEcqpW?=
 =?us-ascii?Q?5gFAy5PFWYSNpGwp6Wfs5l7VGHiX9Q9ZIQtz/F3eqWkQ+89ka58+T4FX8xG6?=
 =?us-ascii?Q?oDkW3glBuVxdP4Q1l+BUfUMgyD5uyEODBQj2xABk7hnNzL1QG0PiecNxwzXm?=
 =?us-ascii?Q?lNqZHC5I1cdSA5OfmD4QGjcTqAm+uQwdAhPWXfR7DUaIPDdgi3CzG3fwV5iC?=
 =?us-ascii?Q?zg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 56b2c8f0-f3e0-4703-b377-08db009f8992
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Jan 2023 19:49:01.2668
 (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: 1cvN643lfzMPSOpYTYkMqH4FwYRy+wD3tnNx/hs5GQFoYHTe98fVIVoW3ICT8K2UWpFVXYe6bCrS4U30yanwfnneSgkxvoaUYkkih3196TE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB7915
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Robert Richter wrote:
> Commit 2aeaf663b85e introduced strict checking for variable length
> payload size validation. The payload length of received data must
> match the size of the requested data by the caller except for the case
> where the min_out value is set.
> 
> The Get Log command does not have a header with a length field set.
> The Log size is determined by the Get Supported Logs command (CXL 3.0,
> 8.2.9.5.1). However, the actual size can be smaller and the number of
> valid bytes in the payload output must be determined reading the
> Payload Length field (CXL 3.0, Table 8-36, Note 2).
> 
> Two issues arise: The command can successfully complete with a payload
> length of zero. And, the valid payload length must then also be
> consumed by the caller.
> 
> Change cxl_xfer_log() to pass the number of payload bytes back to the
> caller to determine the number of log entries. Implement the payload
> handling as a special case where mbox_cmd->size_out is consulted when
> cxl_internal_send_cmd() returns -EIO. A WARN_ONCE() is added to check
> that -EIO is only returned in case of an unexpected output size.
> 
> Logs can be bigger than the maximum payload length and multiple Get
> Log commands can be issued. If the received payload size is smaller
> than the maximum payload size we can assume all valid bytes have been
> fetched. Stop sending further Get Log commands then.
> 
> On that occasion, change debug messages to also report the opcodes of
> supported commands.
> 
> The variable payload commands GET_LSA and SET_LSA are not affected by
> this strict check: SET_LSA cannot be broken because SET_LSA does not
> return an output payload, and GET_LSA never expects short reads.
> 
> Fixes: 2aeaf663b85e ("cxl/mbox: Add variable output size validation for internal commands")
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
> v4:
>  * Fixed a build issue.
> v3:
>  * Added comment to the WARN_ONCE(). (Jonathan)
>  * Passing a size pointer to report the payload size back. (Jonathan)
>  * Moved logging of supported opcodes out of this patch for a separate
>    change. (Jonathan)
> 
>  drivers/cxl/core/mbox.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)

Looks good Robert. Do you think this is v6.2-rc material or can it wait
for v6.3? It sounds like you have a real device that needs, so I am
inclined to add it to cxl/fixes for this cycle.