[PATCH 0/2] cxl: Fix oversized LSA write payload due to header

[PATCH 0/2] cxl: Fix oversized LSA write payload due to header

[Jonathan Cameron]

Fix the bug by claiming the payload is smaller by 8 bytes for both
reads and writes. Also add a defensive check to aid catching similar
bugs in future.

Note that two issues also found in QEMU code whilst looking at this
bug. I'll send out fixes for those shortly.

Note that even with these
ndctl create-namespace fails.  More debugging underway.

Jonathan Cameron (2):
  cxl/mbox: Add a check on input payload size
  cxl/pmem: Fix failure to account for 8 byte header for writes to the
    device LSA.

 drivers/cxl/core/mbox.c | 2 +-
 drivers/cxl/pmem.c      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.32.0

[PATCH 1/2] cxl/mbox: Add a check on input payload size

[Jonathan Cameron]

A bug in the LSA code resulted in transfers slightly larger
than the mailbox size. Let us make it easier to catch similar
issues in future by adding a low level check.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/cxl/core/mbox.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 16176b9278b4..0c90f13870a4 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -174,7 +174,7 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
 	};
 	int rc;
 
-	if (out_size > cxlds->payload_size)
+	if (in_size > cxlds->payload_size || out_size > cxlds->payload_size)
 		return -E2BIG;
 
 	rc = cxlds->mbox_send(cxlds, &mbox_cmd);
-- 
2.32.0

[PATCH 2/2] cxl/pmem: Fix failure to account for 8 byte header for writes to the device LSA.

[Jonathan Cameron]

Writes to the device must include an offset and size as defined in
CXL 2.0 8.2.9.5.2.4 Set LSA (Opcode 4103h)

Fixes tag is non obvious as this code has been through several
reworks and variable names + wasn't in use until the addition
of the region code.

Due to a bug in QEMU CXL emulation this overrun resulted in QEMU
crashing.

Reported-by: Bobo WL <lmw.bobo@gmail.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/cxl/pmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
index 7dc0a2fa1a6b..115a7b79f343 100644
--- a/drivers/cxl/pmem.c
+++ b/drivers/cxl/pmem.c
@@ -107,7 +107,7 @@ static int cxl_pmem_get_config_size(struct cxl_dev_state *cxlds,
 
 	*cmd = (struct nd_cmd_get_config_size) {
 		 .config_size = cxlds->lsa_size,
-		 .max_xfer = cxlds->payload_size,
+		 .max_xfer = cxlds->payload_size - sizeof(struct cxl_mbox_set_lsa),
 	};
 
 	return 0;
-- 
2.32.0

RE: [PATCH 2/2] cxl/pmem: Fix failure to account for 8 byte header for writes to the device LSA.

[Dan Williams]

[ apologies if this is a duplicate response ]

Jonathan Cameron wrote:
> Writes to the device must include an offset and size as defined in
> CXL 2.0 8.2.9.5.2.4 Set LSA (Opcode 4103h)
> 
> Fixes tag is non obvious as this code has been through several
> reworks and variable names + wasn't in use until the addition
> of the region code.

Looks like:

Fixes: 60b8f17215de ("cxl/pmem: Translate NVDIMM label commands to CXL label commands")

...to me since any transfer that got within 8 bytes of the payload_size
would fail.

I notice that cxl_test worked around this bug simply because the mocking
does not attempt to emulate the actual mailbox transfer, just the
logical data transfer. I apprecitate having QEMU to back up the unit
testing.

Re: [PATCH 2/2] cxl/pmem: Fix failure to account for 8 byte header for writes to the device LSA.

[Jonathan Cameron]

On Mon, 15 Aug 2022 14:55:42 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> [ apologies if this is a duplicate response ]
> 
> Jonathan Cameron wrote:
> > Writes to the device must include an offset and size as defined in
> > CXL 2.0 8.2.9.5.2.4 Set LSA (Opcode 4103h)
> > 
> > Fixes tag is non obvious as this code has been through several
> > reworks and variable names + wasn't in use until the addition
> > of the region code.  
> 
> Looks like:
> 
> Fixes: 60b8f17215de ("cxl/pmem: Translate NVDIMM label commands to CXL label commands")
> 
> ...to me since any transfer that got within 8 bytes of the payload_size
> would fail.

Makes sense - go with that one.

> 
> I notice that cxl_test worked around this bug simply because the mocking
> does not attempt to emulate the actual mailbox transfer, just the
> logical data transfer. I apprecitate having QEMU to back up the unit
> testing.
> 

Re: [PATCH 2/2] cxl/pmem: Fix failure to account for 8 byte header for writes to the device LSA.

[Jonathan Cameron]

On Wed, 17 Aug 2022 12:29:30 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Mon, 15 Aug 2022 14:55:42 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > [ apologies if this is a duplicate response ]
> > 
> > Jonathan Cameron wrote:  
> > > Writes to the device must include an offset and size as defined in
> > > CXL 2.0 8.2.9.5.2.4 Set LSA (Opcode 4103h)
> > > 
> > > Fixes tag is non obvious as this code has been through several
> > > reworks and variable names + wasn't in use until the addition
> > > of the region code.    
> > 
> > Looks like:
> > 
> > Fixes: 60b8f17215de ("cxl/pmem: Translate NVDIMM label commands to CXL label commands")
> > 
> > ...to me since any transfer that got within 8 bytes of the payload_size
> > would fail.  
> 
> Makes sense - go with that one.

Hi Dan,

I was assuming you'd pick this up as b4 will happily pick the fixes
tag out of the thread and there weren't any other comments.

Let me know if you want me to resend with the tag in place.

Thanks,

Jonathan
 
> 
> > 
> > I notice that cxl_test worked around this bug simply because the mocking
> > does not attempt to emulate the actual mailbox transfer, just the
> > logical data transfer. I apprecitate having QEMU to back up the unit
> > testing.
> >   
> 

Re: [PATCH 2/2] cxl/pmem: Fix failure to account for 8 byte header for writes to the device LSA.

[Dan Williams]

Jonathan Cameron wrote:
> On Wed, 17 Aug 2022 12:29:30 +0100
> Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> > On Mon, 15 Aug 2022 14:55:42 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> > 
> > > [ apologies if this is a duplicate response ]
> > > 
> > > Jonathan Cameron wrote:  
> > > > Writes to the device must include an offset and size as defined in
> > > > CXL 2.0 8.2.9.5.2.4 Set LSA (Opcode 4103h)
> > > > 
> > > > Fixes tag is non obvious as this code has been through several
> > > > reworks and variable names + wasn't in use until the addition
> > > > of the region code.    
> > > 
> > > Looks like:
> > > 
> > > Fixes: 60b8f17215de ("cxl/pmem: Translate NVDIMM label commands to CXL label commands")
> > > 
> > > ...to me since any transfer that got within 8 bytes of the payload_size
> > > would fail.  
> > 
> > Makes sense - go with that one.
> 
> Hi Dan,
> 
> I was assuming you'd pick this up as b4 will happily pick the fixes
> tag out of the thread and there weren't any other comments.
> 
> Let me know if you want me to resend with the tag in place.

I believe you should have gotten a notice from patchwork, but I have
this queued up for v6.1-rc fixes.