Received-SPF: pass (google.com: domain of linux-kernel+bounces-130005-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 3 Apr 2024 07:53:24 -0700
From: Dan Williams <dan.j.williams@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 v2 1/1] cxl/core: Fix initialization of mbox_cmd.size_out
 in get event
Message-ID: <660d6d64a50bd_7702a29466@dwillia2-xfh.jf.intel.com.notmuch>
References: <20240402081404.1106-1-kwangjin.ko@sk.com>
 <20240402081404.1106-2-kwangjin.ko@sk.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240402081404.1106-2-kwangjin.ko@sk.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?QY2D71+T92nge6a5axiOnwEeQ7AXJVYCmiClTOTORoFkSZ5N5qBl/EEYlHi/?=
 =?us-ascii?Q?KxBlE254csR4xRfk1pTfcnVti5jTo0jIF1QpCatVT7tM+3V7NnYX8Sjl/TIU?=
 =?us-ascii?Q?Pg9TJX7hZR8hyoEQnXFq4pVnmurWJ6BtCRtTMlMHORalo9VDyXS2q7fOEHhd?=
 =?us-ascii?Q?UAfGO3XZJf+yhc+sei5x0dED/Z1QFi/GYSf6cxcI8tWR2kemWgjDTeQLPzNt?=
 =?us-ascii?Q?Bf7uTUcKJ6FetZClRu2AYEnI3rtzuOmPlmBwDJcB2k4gOX11/l4UvYFMDC7F?=
 =?us-ascii?Q?cWyr2clcPV1n9NrzeQlabreTD3B7tbHv0Cro0UgaRP/h8vX4s9s2O164ypYd?=
 =?us-ascii?Q?EjUEU6fsTEhDkxT705UralITr+oKkGSdhcyd5b3uKpkShJ3rVM6nAK7ADEic?=
 =?us-ascii?Q?DWQJ3afSVk8GuGM6WVI3EX4VSzCpLR9sU1A0091PJCy/N5E2qmXPkGYRrH07?=
 =?us-ascii?Q?eIqQLqWS8Xgbk1NgAcCJs4LzWhCnq/iUQuFC3/SSD7WmWFVX1/xufCFUddwK?=
 =?us-ascii?Q?RMR/Qqfvp+nIx0MGlPA43eYE8dusgtBm2Gq9vww9L0dHYVYKxRgQz4Jpoh/E?=
 =?us-ascii?Q?f6p7IxWpIHcDL1zjBt8bao9bG3uw7GZHmJni/Zhml0Iwul/5P89G7yHCe9nr?=
 =?us-ascii?Q?+nLmBBjuuQi/V/wAldB6PygwPc18okt6HYKuJgtzfJdD9NO1x+1d1tfWtir9?=
 =?us-ascii?Q?VCd7guIOxZtYB9VIsgc8vVOU5wlDwxkBHak2pW2MoP8gAuLjqds8J9/+uNsY?=
 =?us-ascii?Q?O6QYDaQcoK2sOjvSS2Vj8k9qpJdoA/KUrC5F82XbduzV+AomTl/6Zyf5QIfD?=
 =?us-ascii?Q?pAwPhlShx5nZ6zSCP1gWriG+xn9TaMPMHz+JMYBd9dnLSBeLNU0IIlIrQC0E?=
 =?us-ascii?Q?7v8mSkjlCLOaYUAfbiVLatmToeXovNvR7LFeZsuPuEb9H8CNZHc/zMgwt9dY?=
 =?us-ascii?Q?uqaDo+OsgpljinV7lp1qg8mHYqa3/MDsoWQtufXSRpYu8w+VoNg18vToQrQd?=
 =?us-ascii?Q?zf+rcgzauAKRROmCc/BQKhh7qYSZrG4jBf6wKbgFnmpRy4mwlrYiIytcqQuF?=
 =?us-ascii?Q?M+48FNG6FJFKw1g5IsvP1b1TWAR6IDTiJg/N2O+NJCfY69gZl40xnGykMME1?=
 =?us-ascii?Q?8nSUZm8kBfEr3+6kXcbnQbYRvBdj0TTt2mRlzVUyZp3iJRy40V682ZJFjHmu?=
 =?us-ascii?Q?V+z2CQxhCcPheHrNehEaDeBh2PkEiWII8p0w5TLfg39LsyPphPW/F6E2wooD?=
 =?us-ascii?Q?WwFc/8kOeps+AeNsH5QWMYJO3l6vBHiuKDj0H0LPEmlIE/WdOfBtRIfmWK31?=
 =?us-ascii?Q?G0wEY9DBccvGrG3M+Y7aIFALv01wgjc/PX9nl3oM81Gdn37eLOrBcfsAZ6+y?=
 =?us-ascii?Q?KxmmLr5YM9Vdu2yuMPKm8N4LmUWyq5tkl1GpHco96OH4k/FHw9QrLhJ39z89?=
 =?us-ascii?Q?COEuAYmCZiWUXp7m6I30zPL+Ms3nrLyWfYZIg5qe+pdGPlFUeqN7WxifFJUh?=
 =?us-ascii?Q?xPZiciHDHe9Y9gWf0GisKFO4TWyDNxA65EOVVIKCKtF82G0Tw+rG5nD5lwtH?=
 =?us-ascii?Q?TTxMgI7s0ZT5++ZtDIOeeGhxw2DASXUxVYwqSe4q7bBgYyMAVhh1OhirBwFj?=
 =?us-ascii?Q?uQ=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6aeb8ad5-ee84-47d7-262e-08dc53edd17d
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Apr 2024 14:53:26.8371
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 64NUsuGuyeE5E3Bx0+klIGFNF+3HNjbV79ncF0tJBf/0EYa502fygsymtI8JzsAExu0ikwCOXME8Qtfw619rxtbov7N/ihG4oZ5dPZh/7KQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB7193
X-OriginatorOrg: intel.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 reading.
> 3) Read the new *two* events.
>    - size_out is still set to 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.
>    - Accessing records[1] will result in reading incorrect data.
> 
> 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;
> +

Fix looks correct, but I am concerned it is a band-aid for a more
general problem. For example, if I am not mistaken, we have a similar
bug in cxl_mem_get_poison().

So perhaps a convention to always define @mbox_cmd immediately before
cxl_internal_send_cmd() like this:

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 9adda4795eb7..5d44b5c095b7 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -946,24 +946,22 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
        struct cxl_memdev *cxlmd = mds->cxlds.cxlmd;
        struct device *dev = mds->cxlds.dev;
        struct cxl_get_event_payload *payload;
-       struct cxl_mbox_cmd mbox_cmd;
        u8 log_type = type;
        u16 nr_rec;
 
        mutex_lock(&mds->event.log_lock);
        payload = mds->event.buf;
 
-       mbox_cmd = (struct cxl_mbox_cmd) {
-               .opcode = CXL_MBOX_OP_GET_EVENT_RECORD,
-               .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;
+               struct cxl_mbox_cmd mbox_cmd = (struct cxl_mbox_cmd){
+                       .opcode = CXL_MBOX_OP_GET_EVENT_RECORD,
+                       .payload_in = &log_type,
+                       .size_in = sizeof(log_type),
+                       .payload_out = payload,
+                       .size_out = mds->payload_size,
+                       .min_out = struct_size(payload, records, 0),
+               };
 
                rc = cxl_internal_send_cmd(mds, &mbox_cmd);
                if (rc) {
@@ -1296,7 +1294,6 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
        struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
        struct cxl_mbox_poison_out *po;
        struct cxl_mbox_poison_in pi;
-       struct cxl_mbox_cmd mbox_cmd;
        int nr_records = 0;
        int rc;
 
@@ -1308,16 +1305,16 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
        pi.offset = cpu_to_le64(offset);
        pi.length = cpu_to_le64(len / CXL_POISON_LEN_MULT);
 
-       mbox_cmd = (struct cxl_mbox_cmd) {
-               .opcode = CXL_MBOX_OP_GET_POISON,
-               .size_in = sizeof(pi),
-               .payload_in = &pi,
-               .size_out = mds->payload_size,
-               .payload_out = po,
-               .min_out = struct_size(po, record, 0),
-       };
-
        do {
+               struct cxl_mbox_cmd mbox_cmd = (struct cxl_mbox_cmd){
+                       .opcode = CXL_MBOX_OP_GET_POISON,
+                       .size_in = sizeof(pi),
+                       .payload_in = &pi,
+                       .size_out = mds->payload_size,
+                       .payload_out = po,
+                       .min_out = struct_size(po, record, 0),
+               };
+
                rc = cxl_internal_send_cmd(mds, &mbox_cmd);
                if (rc)
                        break;