[PATCH 1/2] cxl/core: Fix initialization of mbox_cmd.size_out in get event

[PATCH 1/2] cxl/core: Fix initialization of mbox_cmd.size_out in get event

[Kwangjin Ko]

Since mbox_cmd.size_out is overwritten with the actual output size in
the function below, it needs to be initialized every time.

cxl_internal_send_cmd -> __cxl_pci_mbox_send_cmd

Problem scenario:

1) The size_out variable is initially set to the size of the mailbox
2) Read an event
   - size_out is set to 160 bytes (header 32B + one event 128B)
   - Two event are created while read
3) Read two events
   - size_out is still set as 160 bytes
   - Although the value of out_len is 288 bytes, only 160 bytes are
     copied from the mailbox register to the local variable
   - record_count is set to 2
   - Acessing records[1] causes a buffer overflow

Signed-off-by: Kwangjin Ko <kwangjin.ko@sk.com>
---
 drivers/cxl/core/mbox.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 9adda4795eb7..a38531a055c8 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -958,13 +958,14 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
 		.payload_in = &log_type,
 		.size_in = sizeof(log_type),
 		.payload_out = payload,
-		.size_out = mds->payload_size,
 		.min_out = struct_size(payload, records, 0),
 	};
 
 	do {
 		int rc, i;
 
+		mbox_cmd.size_out = mds->payload_size;
+
 		rc = cxl_internal_send_cmd(mds, &mbox_cmd);
 		if (rc) {
 			dev_err_ratelimited(dev,

base-commit: 39cd87c4eb2b893354f3b850f916353f2658ae6f
-- 
2.34.1

[PATCH 2/2] cxl/core: Fix incorrect array index in cxl_clear_event_record()

[Kwangjin Ko]

The variable 'i' should be incremented after dev_dbg(), or print
gen->hdr.handle instead. Otherwise, the handle value of the next loop
will be printed. Furthermore, in the final loop, accessing 'handles[i]'
will cause a buffer overflow.

Signed-off-by: Kwangjin Ko <kwangjin.ko@sk.com>
---
 drivers/cxl/core/mbox.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index a38531a055c8..84014aa01c95 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -913,9 +913,9 @@ static int cxl_clear_event_record(struct cxl_memdev_state *mds,
 		struct cxl_event_record_raw *raw = &get_pl->records[cnt];
 		struct cxl_event_generic *gen = &raw->event.generic;
 
-		payload->handles[i++] = gen->hdr.handle;
 		dev_dbg(mds->cxlds.dev, "Event log '%d': Clearing %u\n", log,
-			le16_to_cpu(payload->handles[i]));
+			le16_to_cpu(gen->hdr.handle));
+		payload->handles[i++] = gen->hdr.handle;
 
 		if (i == max_handles) {
 			payload->nr_recs = i;
-- 
2.34.1

Re: [PATCH 2/2] cxl/core: Fix incorrect array index in cxl_clear_event_record()

[Alison Schofield]

On Mon, Apr 01, 2024 at 06:10:56PM +0900, Kwangjin Ko wrote:
> The variable 'i' should be incremented after dev_dbg(), or print
> gen->hdr.handle instead. Otherwise, the handle value of the next loop
> will be printed. Furthermore, in the final loop, accessing 'handles[i]'
> will cause a buffer overflow.

This fix is already queued:
https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/?h=fixes

> 
> Signed-off-by: Kwangjin Ko <kwangjin.ko@sk.com>
> ---
>  drivers/cxl/core/mbox.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index a38531a055c8..84014aa01c95 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -913,9 +913,9 @@ static int cxl_clear_event_record(struct cxl_memdev_state *mds,
>  		struct cxl_event_record_raw *raw = &get_pl->records[cnt];
>  		struct cxl_event_generic *gen = &raw->event.generic;
>  
> -		payload->handles[i++] = gen->hdr.handle;
>  		dev_dbg(mds->cxlds.dev, "Event log '%d': Clearing %u\n", log,
> -			le16_to_cpu(payload->handles[i]));
> +			le16_to_cpu(gen->hdr.handle));
> +		payload->handles[i++] = gen->hdr.handle;
>  
>  		if (i == max_handles) {
>  			payload->nr_recs = i;
> -- 
> 2.34.1
> 

Re: [PATCH 1/2] cxl/core: Fix initialization of mbox_cmd.size_out in get event

[Ira Weiny]

Kwangjin Ko wrote:
> Since mbox_cmd.size_out is overwritten with the actual output size in
> the function below, it needs to be initialized every time.
> 
> cxl_internal_send_cmd -> __cxl_pci_mbox_send_cmd
> 
> Problem scenario:
> 
> 1) The size_out variable is initially set to the size of the mailbox
> 2) Read an event
>    - size_out is set to 160 bytes (header 32B + one event 128B)
>    - Two event are created while read
> 3) Read two events
>    - size_out is still set as 160 bytes
>    - Although the value of out_len is 288 bytes, only 160 bytes are
>      copied from the mailbox register to the local variable
>    - record_count is set to 2
>    - Acessing records[1] causes a buffer overflow

I see how this causes a missed event due to only 1 event being copied into
the buffer but how does this cause a buffer overflow with the payload
buffer being set to the max mailbox size?

Did you simply mean that records[1] accesses incorrect data?  I agree that
is an issue.  I'm looking at a test for this.

Ira

> 
> Signed-off-by: Kwangjin Ko <kwangjin.ko@sk.com>
> ---
>  drivers/cxl/core/mbox.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 9adda4795eb7..a38531a055c8 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -958,13 +958,14 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
>  		.payload_in = &log_type,
>  		.size_in = sizeof(log_type),
>  		.payload_out = payload,
> -		.size_out = mds->payload_size,
>  		.min_out = struct_size(payload, records, 0),
>  	};
>  
>  	do {
>  		int rc, i;
>  
> +		mbox_cmd.size_out = mds->payload_size;
> +
>  		rc = cxl_internal_send_cmd(mds, &mbox_cmd);
>  		if (rc) {
>  			dev_err_ratelimited(dev,
> 
> base-commit: 39cd87c4eb2b893354f3b850f916353f2658ae6f
> -- 
> 2.34.1
>