From: Ira Weiny <ira.weiny@intel.com>
To: Kwangjin Ko <kwangjin.ko@sk.com>, <dave@stgolabs.net>,
<jonathan.cameron@huawei.com>, <dave.jiang@intel.com>,
<alison.schofield@intel.com>, <vishal.l.verma@intel.com>,
<ira.weiny@intel.com>, <dan.j.williams@intel.com>
Cc: <linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
<kernel_team@skhynix.com>
Subject: Re: [PATCH 1/2] cxl/core: Fix initialization of mbox_cmd.size_out in get event
Date: Mon, 1 Apr 2024 13:26:46 -0700 [thread overview]
Message-ID: <660b1886315db_750ac294bc@iweiny-mobl.notmuch> (raw)
In-Reply-To: <20240401091057.1044-1-kwangjin.ko@sk.com>
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
>
prev parent reply other threads:[~2024-04-01 20:26 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
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 ` Ira Weiny [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=660b1886315db_750ac294bc@iweiny-mobl.notmuch \
--to=ira.weiny@intel.com \
--cc=alison.schofield@intel.com \
--cc=dan.j.williams@intel.com \
--cc=dave.jiang@intel.com \
--cc=dave@stgolabs.net \
--cc=jonathan.cameron@huawei.com \
--cc=kernel_team@skhynix.com \
--cc=kwangjin.ko@sk.com \
--cc=linux-cxl@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=vishal.l.verma@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).