Received-SPF: pass (google.com: domain of linux-kernel+bounces-127027-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 1 Apr 2024 13:26:46 -0700
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
Message-ID: <660b1886315db_750ac294bc@iweiny-mobl.notmuch>
References: <20240401091057.1044-1-kwangjin.ko@sk.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240401091057.1044-1-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?qCTywhjYp6G9K8a246VT1cQcV7mhOXqPw6z+PUDr45Ujmc+6HlvRItJC4rJU?=
 =?us-ascii?Q?U8pXM7P/iO4nOdB2RaFck5URhU4YMBu8jSVzidvs5aoq8ojqJ2gfFLDgyGk3?=
 =?us-ascii?Q?MHz+Dp4Ss+f2Kh6t0NyqAfp1BbRPNiuzOi5o1KQVqVvL9EvJqYBbLGcDgqbj?=
 =?us-ascii?Q?nMh5TXKN9JfQMX6bPZbzPzmLAuSx5GSQu6Z4DCB/7WxZ7iRnPLroMyOFcqhe?=
 =?us-ascii?Q?fwNLRz8LVe6VRyrWwPqFPaCH8udUuTgxMMNAXaliF+XvNIHZSGeyM4fMj0Aj?=
 =?us-ascii?Q?4BW4YqoiWEdJG7KyYiK6kjye8nzguWrG+ilkP4ubkHC/HQcI1Hv8iyTQuBS7?=
 =?us-ascii?Q?wRqwp0kCX8IlFyziAu4T0RGNhjON5LaR8zC5hyYFt6mw+jcSRLV2KCdrADw1?=
 =?us-ascii?Q?ho0Pu8nPpYn5zdIX5HX5SGmF3YIeVlZxkka3GnDyR/P5IEydlaejPb6vfTTy?=
 =?us-ascii?Q?Xec6kqsHhZNMdHoKpEGqaKDR+16rEMSUjB33LIdmzQftFeGYQ8N4yPH4Lj9t?=
 =?us-ascii?Q?MiuoB/PNuHrtl00sZ++igt3tqShJphIUw4Fz+WxIrkbaacqbWXxclrBaiAEp?=
 =?us-ascii?Q?uqm1tXxVkW4ljb2WTNBoSnO8XOX0/Bjy+sw4am/jlIxwTLKta6ZU41MCK3Ix?=
 =?us-ascii?Q?vUrqlXXWDulOzdT3MqLkxSEkYfXWxyccFdNB3J3MOLa4l9Bq5XcpM66kJb9C?=
 =?us-ascii?Q?DwjiktBFEH3GqgsM9YB0x8B0pwOWFL386S2VmMucsSGE5fQ3NKDPCxOQvFmB?=
 =?us-ascii?Q?+UkG+b07ijktpsT7aO92YeyLRLeDoyZgCKWNl8Nn4q5ODyDR41H1zJVTmN9a?=
 =?us-ascii?Q?46B/kjSrgR+UEDDXBiop7YHFUTzUxTOWsbXah8IX60hMu8+m72BuDo2WvhSn?=
 =?us-ascii?Q?0P6tfdxOvjjFDJxL2TMEE1jUEhKykKMfsv+DobcTt5ScH2U24TI1qf4Ol96O?=
 =?us-ascii?Q?sAb5syLrDe8CAyFCo85ks1k80GVqQddrBcnKo1wJ+/GQjLRE6bI/Y1k1LiPn?=
 =?us-ascii?Q?0BYv1NNjCzeFLxl1IAo33cdzm6FLYKfe3R7vgr40d7z6O8uJtCgSNZk3xF5M?=
 =?us-ascii?Q?4V9dNCZtznYGHMes9mTlRZ2pvFCYYKt1DV7yb+Zxbam6qkZYShCtviJZKIgk?=
 =?us-ascii?Q?EEoE6Msf/Ws4TuIvMUtlnC0zZbrP7CabPz3esNDEDGwjnOC9JQrqcA1MUfja?=
 =?us-ascii?Q?zuopPvZN5/Qj+kOA27p7zPupbgsnFkcyeLC2y6in9pJmWAqRsXUYbwgrl4VC?=
 =?us-ascii?Q?ndHLaNeqzk3N5G3suPBTM45q48/gFjBnDXlloIdqHfwk8N0a1vl/Jk3kWk57?=
 =?us-ascii?Q?/zdQgJd1RM9bvxxkb0VDP8kvpz57mTnRW08dOd15OgxAJsa0qcrfXRVnmeTt?=
 =?us-ascii?Q?rYzlntK6iR9DKN0Cse9qcde2PYi2Qwz9HnSORqJ4o9pO1ypghBsrkfqXCPBP?=
 =?us-ascii?Q?wD3AEnFQPPk5s+OvrJfsKGRghNKhWNaussXjADhOOW483iYOuUPJMSYB73KM?=
 =?us-ascii?Q?vbhDmiicmcywGf3+fnLifQ91Jb51AJe3PlU/lA/FHhEUaSmUmer+bYmaZxT2?=
 =?us-ascii?Q?5dXrdB5y0/vKGXRmwdCyqiu8joGJTIoe7dRVsT1C?=
X-MS-Exchange-CrossTenant-Network-Message-Id: fb34a45f-483b-4874-082d-08dc528a1202
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Apr 2024 20:26:54.2724
 (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: wY1pJqeeiI+SYb6Xa68MXG2uCAesKz/PQ2s85MoVXm2pTJROj8Txm6KOrTXfBOS3X2Qp02tDT0vbpzrCO8kqcQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV3PR11MB8693
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 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
>