* [PATCH 1/2] cxl/core: Fix initialization of mbox_cmd.size_out in get event
@ 2024-04-01 9:10 Kwangjin Ko
2024-04-01 9:10 ` [PATCH 2/2] cxl/core: Fix incorrect array index in cxl_clear_event_record() Kwangjin Ko
2024-04-01 20:26 ` [PATCH 1/2] cxl/core: Fix initialization of mbox_cmd.size_out in get event Ira Weiny
0 siblings, 2 replies; 4+ messages in thread
From: Kwangjin Ko @ 2024-04-01 9:10 UTC (permalink / raw)
To: dave, jonathan.cameron, dave.jiang, alison.schofield,
vishal.l.verma, ira.weiny, dan.j.williams
Cc: linux-cxl, linux-kernel, kernel_team
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] cxl/core: Fix incorrect array index in cxl_clear_event_record()
2024-04-01 9:10 [PATCH 1/2] cxl/core: Fix initialization of mbox_cmd.size_out in get event Kwangjin Ko
@ 2024-04-01 9:10 ` Kwangjin Ko
2024-04-01 19:09 ` Alison Schofield
2024-04-01 20:26 ` [PATCH 1/2] cxl/core: Fix initialization of mbox_cmd.size_out in get event Ira Weiny
1 sibling, 1 reply; 4+ messages in thread
From: Kwangjin Ko @ 2024-04-01 9:10 UTC (permalink / raw)
To: dave, jonathan.cameron, dave.jiang, alison.schofield,
vishal.l.verma, ira.weiny, dan.j.williams
Cc: linux-cxl, linux-kernel, kernel_team
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] cxl/core: Fix incorrect array index in cxl_clear_event_record()
2024-04-01 9:10 ` [PATCH 2/2] cxl/core: Fix incorrect array index in cxl_clear_event_record() Kwangjin Ko
@ 2024-04-01 19:09 ` Alison Schofield
0 siblings, 0 replies; 4+ messages in thread
From: Alison Schofield @ 2024-04-01 19:09 UTC (permalink / raw)
To: Kwangjin Ko
Cc: dave, jonathan.cameron, dave.jiang, vishal.l.verma, ira.weiny,
dan.j.williams, linux-cxl, linux-kernel, kernel_team
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] cxl/core: Fix initialization of mbox_cmd.size_out in get event
2024-04-01 9:10 [PATCH 1/2] cxl/core: Fix initialization of mbox_cmd.size_out in get event Kwangjin Ko
2024-04-01 9:10 ` [PATCH 2/2] cxl/core: Fix incorrect array index in cxl_clear_event_record() Kwangjin Ko
@ 2024-04-01 20:26 ` Ira Weiny
1 sibling, 0 replies; 4+ messages in thread
From: Ira Weiny @ 2024-04-01 20:26 UTC (permalink / raw)
To: Kwangjin Ko, dave, jonathan.cameron, dave.jiang,
alison.schofield, vishal.l.verma, ira.weiny, dan.j.williams
Cc: linux-cxl, linux-kernel, kernel_team
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-04-01 20:26 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-01 9:10 [PATCH 1/2] cxl/core: Fix initialization of mbox_cmd.size_out in get event Kwangjin Ko
2024-04-01 9:10 ` [PATCH 2/2] cxl/core: Fix incorrect array index in cxl_clear_event_record() Kwangjin Ko
2024-04-01 19:09 ` Alison Schofield
2024-04-01 20:26 ` [PATCH 1/2] cxl/core: Fix initialization of mbox_cmd.size_out in get event Ira Weiny
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).