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;
Received-SPF: Pass (protection.outlook.com: domain of amd.com designates
 165.204.84.17 as permitted sender) receiver=protection.outlook.com;
 client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C
Date:   Wed, 4 Jan 2023 12:31:54 +0100
From:   Robert Richter <rrichter@amd.com>
To:     Dan Williams <dan.j.williams@intel.com>
CC:     Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Ira Weiny <ira.weiny@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        Dave Jiang <dave.jiang@intel.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: <Y7VjfRZlBDnHewAd@rric.localdomain>
References: <20230103205937.1126626-1-rrichter@amd.com>
 <63b4a8153d6b6_517412948a@dwillia2-xfh.jf.intel.com.notmuch>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <63b4a8153d6b6_517412948a@dwillia2-xfh.jf.intel.com.notmuch>
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jan 2023 11:32:00.1616
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 19883057-fbdb-43fb-e881-08daee474b6d
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com]
X-MS-Exchange-CrossTenant-AuthSource: BN8NAM11FT094.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR12MB7684
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS 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

On 03.01.23 14:11:33, Dan Williams wrote:
> 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.

> 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?

The spec is not really clear here and I have seen a CXL device
firmware implementation that interprets it like that. We could demand
a firmware fix for that, but the kernel driver would be more robust if
we lower the strictness here.

Reading the spec again I just found that "the maximum size of each
Log" is mentioned in the description:

"""
8.2.9.5.1 Get Supported Logs (Opcode 0400h)

Retrieve the list of device specific logs (identified by UUID) and
the maximum size of each Log.
"""

With that and the note in Table 8-36 stating that the exact payload of
a variable length command should be determined using the Payload
Length field, I think the commands can return different payload
lengths.

> > 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.

Ok, I haven't checked the details here but thought it is worth to
note.

> 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.

I will prepare a v2 with that change included.

Thanks,

-Robert