| Message ID | 20240401091057.1044-1-kwangjin.ko@sk.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] cxl/core: Fix initialization of mbox_cmd.size_out in get event | expand |
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 >
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,
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(-) base-commit: 39cd87c4eb2b893354f3b850f916353f2658ae6f