2024-04-01 09:11:47

by Kwangjin Ko

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

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 <[email protected]>
---
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

2024-04-01 20:27:23

by Ira Weiny

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

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 <[email protected]>
> ---
> 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
>