Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 3 Jan 2023 14:11:33 -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:     Robert Richter <rrichter@amd.com>, <linux-cxl@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] cxl/mbox: Fix Payload Length check for Get Log command
Message-ID: <63b4a8153d6b6_517412948a@dwillia2-xfh.jf.intel.com.notmuch>
References: <20230103205937.1126626-1-rrichter@amd.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230103205937.1126626-1-rrichter@amd.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?U2ueeX7E2yVIQXxR7mvDLPGbWaC91VrO4dba9QjS2iE1ZuePx7hl+Q4JDu/F?=
 =?us-ascii?Q?8oG2rWS8gM32evidwXtaoKgWz3q/SqgUxEdZR4bFb84GMLXebt45vd+N2Ui5?=
 =?us-ascii?Q?goqg933AsRci2U7yDaXIHB1oNSk397jOC8ML4Uw1620FdknaDNF9J7DOpM4M?=
 =?us-ascii?Q?s2D/K14TR+FjmKBNEKKWbre910CA+lroSPMd974PTQb3btRgP0DtWM1swEle?=
 =?us-ascii?Q?s40ZOyzYgbXRQxPXne/3rGWEPSowc/NEOzkyh2gldVfV9e6Pkw7VOk8fyULW?=
 =?us-ascii?Q?HFCEGGenPL8VYg67aB2Wl+dQMmHb+9GxhaReXc7f5QMBrxav1uFOIm4BiR7G?=
 =?us-ascii?Q?NR/1vcC2fCieVQV5y5zlnSJie+eOuTtsSe7DuCFpJHbK8GtTuxCyuk02q1L5?=
 =?us-ascii?Q?PW2POpXen103dE6b97+8Kam9DFErZwBzdZVJOGAuzAJ1N2XtCfgOTQCvgqQg?=
 =?us-ascii?Q?DU88nOruBdoK+ktLD98KX1hMutaz+MEYeYoIeuxwP7O3ZvJhfT3100tetwYq?=
 =?us-ascii?Q?njWLha66tzpDWBhWxK2Tm2Hgh91UQ71aA1QEr2f7bnuuhtuH8pUNzki0/e/y?=
 =?us-ascii?Q?3yEF5lWtq7a4pMr9T/U5HJzbhiM0tbKQiZug2GBywT5mLn/fsd2NYE6w/yQQ?=
 =?us-ascii?Q?h35a2pyB+gChBeX+sDyZQkdDYjV3oR4iXHf+YsmPhDeGBnmyVMJ788aQC3ac?=
 =?us-ascii?Q?RhHUUhclj1U10VMP47m6R/FWid8D3F7NaCQN6noWacgb6agMN/hiOvz2BhGk?=
 =?us-ascii?Q?opoQlfWzuBOaMeLWXWrA2BGdy3kevJLtAo/feGZv2YxJ1zA0h8YRp7rs+xcb?=
 =?us-ascii?Q?aShINJ30wNDdoKw38XHz6aXtywr3ysG45feVdNaITTddW9XuU1b+hN0F1Uca?=
 =?us-ascii?Q?rg8CkTaP1U7AvmP+GmL2fMC4K7OhL0c5FmWPjJaPOye1DNZnfum0y5J9qC/a?=
 =?us-ascii?Q?ZlIet6XkFathI7WUHw2sya1n5Yc74gVnSq3Jnb0LhcCGipy26LrkLrxRtXmu?=
 =?us-ascii?Q?kJT9PTAtJAcF8elfp5beo9la5b+YikW9w/dRhmHmOlacjz0VDWXwU/g6MSsg?=
 =?us-ascii?Q?AGYdq75xIT9aMtQvdzUq/rDTNxm/nGpOvEicdslSt4FyErvris8tbX/JT3ix?=
 =?us-ascii?Q?RWl0tMifuS1FSN0Wxm22fUb4WKvPCt/f8s5DSz+dzkJJlIuEr5tLREemgCNX?=
 =?us-ascii?Q?MOsOIss2SsaHWeJDUJjqx9pgq6o+UqINiPCTcAqt5KNtwJnGGSHz+KfveYlq?=
 =?us-ascii?Q?d9vASXpaDfXmgS74091FH4ZevKUVu6XwuoyUZEvnJS19WfJnLsQfgsBmFpgI?=
 =?us-ascii?Q?OXXgc82PA1+Fog4Fu+DNhwP8l8m6GABxR9xCIY7NOpqhO8DJrIbT2S+cEPQW?=
 =?us-ascii?Q?jc2KTDqIzCAzblTyKgbzjx+lZn5ZqOiGBRP+p7TJWCt9LflC7XVH4C4LQ6S0?=
 =?us-ascii?Q?zSmF6//tYZd3soCvflmio9o1C9xAJ24VA3tF+R+15x1JKnC4+cnKjcWvygLY?=
 =?us-ascii?Q?2e2cT9jl2iCV8BnmpVrLllbilwLbvR0U+1qqJ+hWZ9NjJ4WocErauwj1uHw2?=
 =?us-ascii?Q?zEfQ4eH/VaPjJRAFBtYLCVhGA9ZPtorjtjqSRY16e3AeK2CZio8Hv1mY6UP9?=
 =?us-ascii?Q?TA=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4fbb5bff-3061-4f73-74c9-08daedd77a63
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1101MB2126.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Jan 2023 22:11:35.6248
 (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: iVabQFp6D1qXdimmJSgAaTU2A9HyG5PNr/FJkfnj+Z3EIr/VRN/O7gtfRz7WZDCU9mGQHprdmuSA2gFDcG30UMGVEmXAgeZx5U6+WRYqZkU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7608
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
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.

Perhaps this is confusion about what the "Log Size" field of Get
Supported Logs means? My reading is that the "Log Size" field indicates
the data "currently available" in the log. Correct me if I am wrong, but
it seems your reading is that it is the "possibly available" data and
software can not assume anything is available until it actually goes to
read the log.

The spec does feel ambiguous here as it could have left off the word
"maximum" to make it clearer it is the data currently pending value, or
leave off the word "available" to make it clear that this value is the
max that will ever be valid for one retrieval, but not necessarily
currently. I do not think "CXL 3.0, Table 8-36, Note 2" helps to clear
this up either as that does not answer what expectations software can
assume about Get Log::Log Size.

Are you sure that this is not a device-side implementation issue where
it needs to make sure that Get Supported Logs indicates what Get Log can
expect?

Table 8-94 "Identify Memory Device Output Payload" specifies the
*possible* data that can be made available in a log.

> 
> Change cxl_xfer_log() to pass the number of payload bytes back to the
> caller to determine the number of log entries.
> 
> 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.
> 
> Also, implement CXL_NO_PAYLOAD_SIZE_VALIDATION as special value to
> @min_out to skip the payload size validation check. A value of zero
> for @min_out is already widespread in use as default if the size must
> match @size_out. Thus, zero can not be passed to allow zero length
> variable payloads, CXL_NO_PAYLOAD_SIZE_VALIDATION should be used
> instead for this case.
> 
> On that occasion, change debug messages to also report supported
> opcodes.
> 
> There could be other variable payloads commands affected by this
> strict check, the implementation of GET_LSA and SET_LSA in this kernel
> could possibly be broken too. A fix of this is not scope of this
> patch.

SET_LSA cannot be broken because SET_LSA does not return an output
payload, and GET_LSA never expects short reads.

Now, if short reads need to be supported on production devices for any
variable length output payload command, I would rather see that handled
as a cxl_internal_send_cmd() special case where mbox_cmd->size_out is
consulted when cxl_internal_send_cmd() returns -EIO.

> 
> Fixes: 2aeaf663b85e ("cxl/mbox: Add variable output size validation for internal commands")
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/cxl/core/mbox.c | 41 ++++++++++++++++++++++++++++++-----------
>  drivers/cxl/cxlmem.h    |  5 +++++
>  2 files changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index b03fba212799..0c2056ae8aff 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -183,11 +183,16 @@ int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
>  	 * Variable sized output needs to at least satisfy the caller's
>  	 * minimum if not the fully requested size.
>  	 */
> +
> +	if (min_out == CXL_NO_PAYLOAD_SIZE_VALIDATION)
> +		return 0;
> +
>  	if (min_out == 0)
>  		min_out = out_size;
>  
>  	if (mbox_cmd->size_out < min_out)
>  		return -EIO;
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_internal_send_cmd, CXL);
> @@ -554,6 +559,7 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8
>  {
>  	u32 remaining = size;
>  	u32 offset = 0;
> +	u32 size_out;
>  
>  	while (remaining) {
>  		u32 xfer_size = min_t(u32, remaining, cxlds->payload_size);
> @@ -572,6 +578,8 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8
>  			.size_in = sizeof(log),
>  			.payload_in = &log,
>  			.size_out = xfer_size,
> +			/* Any size is allowed (CXL 3.0, Table 8-36). */
> +			.min_out = CXL_NO_PAYLOAD_SIZE_VALIDATION,
>  			.payload_out = out,
>  		};
>  
> @@ -579,12 +587,24 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8
>  		if (rc < 0)
>  			return rc;
>  
> -		out += xfer_size;
> -		remaining -= xfer_size;
> -		offset += xfer_size;
> +		size_out = mbox_cmd.size_out;
> +		if (size_out > xfer_size)
> +			return -ENXIO;
> +
> +		out += size_out;
> +		remaining -= size_out;
> +		offset += size_out;
> +
> +		/*
> +		 * A smaller output payload length indicates all valid
> +		 * bytes have been fetched.
> +		 */
> +		if (size_out < xfer_size)
> +			break;
>  	}
>  
> -	return 0;
> +	/* Total number of bytes fetched. */
> +	return offset;
>  }
>  
>  /**
> @@ -608,13 +628,11 @@ static void cxl_walk_cel(struct cxl_dev_state *cxlds, size_t size, u8 *cel)
>  		u16 opcode = le16_to_cpu(cel_entry[i].opcode);
>  		struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
>  
> -		if (!cmd) {
> -			dev_dbg(cxlds->dev,
> -				"Opcode 0x%04x unsupported by driver", opcode);
> -			continue;
> -		}
> +		if (cmd)
> +			set_bit(cmd->info.id, cxlds->enabled_cmds);
>  
> -		set_bit(cmd->info.id, cxlds->enabled_cmds);
> +		dev_dbg(cxlds->dev, "Opcode 0x%04x %ssupported by driver",
> +			opcode, cmd ? "" : "un");
>  	}
>  }
>  
> @@ -695,11 +713,12 @@ int cxl_enumerate_cmds(struct cxl_dev_state *cxlds)
>  		}
>  
>  		rc = cxl_xfer_log(cxlds, &uuid, size, log);
> -		if (rc) {
> +		if (rc < 0) {
>  			kvfree(log);
>  			goto out;
>  		}
>  
> +		size = (u32)rc;
>  		cxl_walk_cel(cxlds, size, log);
>  		kvfree(log);
>  
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index ab138004f644..2db24b062913 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -102,6 +102,10 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
>   *            variable sized output commands, it tells the exact number of bytes
>   *            written.
>   * @min_out: (input) internal command output payload size validation
> + *  * %0:   Payload size must match @size_out.
> + *  * %>0:  Minimum payload size.
> + *  * %CXL_NO_PAYLOAD_SIZE_VALIDATION: Skip payload size validation check.
> + *
>   * @return_code: (output) Error code returned from hardware.
>   *
>   * This is the primary mechanism used to send commands to the hardware.
> @@ -117,6 +121,7 @@ struct cxl_mbox_cmd {
>  	size_t size_in;
>  	size_t size_out;
>  	size_t min_out;
> +#define CXL_NO_PAYLOAD_SIZE_VALIDATION	SIZE_MAX
>  	u16 return_code;
>  };
>  
> -- 
> 2.30.2
>