linux-kernel - Re: [PATCH] cxl/mbox: Fix Payload Length check for Get Log command

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Y8Fcj+BJuUJmK5sk@rric.localdomain>
Date:   Fri, 13 Jan 2023 14:28:47 +0100
From:   Robert Richter <rrichter@....com>
To:     Dan Williams <dan.j.williams@...el.com>
CC:     Alison Schofield <alison.schofield@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        Ira Weiny <ira.weiny@...el.com>,
        Ben Widawsky <bwidawsk@...nel.org>,
        Dave Jiang <dave.jiang@...el.com>, <linux-cxl@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] cxl/mbox: Fix Payload Length check for Get Log command

Dan,

On 04.01.23 12:31:59, Robert Richter wrote:
> 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.

any opinion here? Looks like the device may send a smaller payload in
the 401h command than the size given in 400h.

I have sent an updated patch last week:

 [PATCH v2] cxl/mbox: Fix Payload Length check for Get Log command
 https://patchwork.kernel.org/project/cxl/patch/20230104202954.1163366-1-rrichter@amd.com/

Please take a look.

Thanks,

-Robert