Received-SPF: pass (google.com: domain of linux-kernel+bounces-162670-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, 29 Apr 2024 09:17:38 -0700
From: Ira Weiny <ira.weiny@intel.com>
To: Dan Carpenter <dan.carpenter@linaro.org>, Li Zhijian
	<lizhijian@fujitsu.com>
CC: <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>, <linux-cxl@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/2] cxl/region: Fix potential invalid pointer dereference
Message-ID: <662fc8227e6ea_19ca3929472@iweiny-mobl.notmuch>
References: <20240429013154.368118-1-lizhijian@fujitsu.com>
 <18a295a2-b734-45dd-9c95-660a9e1f58e4@moroto.mountain>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <18a295a2-b734-45dd-9c95-660a9e1f58e4@moroto.mountain>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?4z7zbc/U0RcGHPxUQtt5/kqj7ilkB8x5BP0oHOhYhfECgbub+asVMx9PA5YG?=
 =?us-ascii?Q?MbrqNkiMD8QloeS6/frotkebaAHkBa04yqRaywWhyUTbXl6cnBWoecyLi6aj?=
 =?us-ascii?Q?WAkM/S0qBm0qiLoqz9+hIOE9K/jbcbAzuWdtt6whY3u8j4PQxbNfN0RDsNgE?=
 =?us-ascii?Q?/5d5rVMokypB6prKAI4Dbdup0CMNI+CcbFu73AXHLcC3RP8vi3F5tNJCK3XO?=
 =?us-ascii?Q?4/gAFL2cUC2zdNrr14fkjfhlLvCGL9d4NccXy6hEx9ig54B9UK/Pl8kAD8b8?=
 =?us-ascii?Q?OJwvN9/o3YMAU/yY5PAwM0AJDy43s79haE8A35ya9DWYSeotAwVAgbcpBYT9?=
 =?us-ascii?Q?oPc2Vl4+jdyIRYnY/Djf+4WKqMe0n5bcQMkBHtUq3a6MIsOBzl51KURIDcpq?=
 =?us-ascii?Q?C2TQ3px8Ut1sEjJZ0qqK3m/HKD5Hehl0nGBAu7rNgyLvBdQ6+BymdDg0uWf6?=
 =?us-ascii?Q?32Wl8u52UgFA48IuwyyCIjiEieMjy8RyCJRKHbaeMVCFCxX3C+SXPMoIctRn?=
 =?us-ascii?Q?EOVWblcjwWdOe0YxAgTnlD+CXOO6zXrFyQ+cpy+Gwx1p5BJKK6+M+xuZVFWe?=
 =?us-ascii?Q?xh7ZC4cv3Lh5xRd6cEoAk+9u9L1hglhRnhz5aW4NIVeq5MJjIQcA75XrGASR?=
 =?us-ascii?Q?TA4SmUnjza1FEf9e5P4Rojtf8RSNkOjCOBDCoP8gEVG3o/N2Y7cXmbBx/acw?=
 =?us-ascii?Q?NoV6v0j7+8gFFCh4h8IfR8YiSo8WhOq/1KY4SEyOQPesLBYBB6ay/s5LiYq3?=
 =?us-ascii?Q?NtX+RJqDl9ipOkmLz/aw2iqfa/IKRPOnHe6CHvyoMM8+Wa9Y2ltkYyKf1EDG?=
 =?us-ascii?Q?uQSSCzJOa5frBblBD/WbMBuQu50jk+CRImHIQ87uhEH+eAtcR20pDCFdWNlo?=
 =?us-ascii?Q?gRHgkybwo514oCipk51nH18q7vXgos/TRAlWHQCnHBjIAOMNky7AlbcLFY++?=
 =?us-ascii?Q?Firp1HWArT2mdUncPZ+EmWOP8EsBZp+OXOyKqRMT9wY6SBjVh6IP1wYxA8LF?=
 =?us-ascii?Q?NQQkG71RYZ2jILiau28rr4/1y7zFGwtvGq2LLpI4HMFaY6VhqTeE4pyvRhu6?=
 =?us-ascii?Q?4sP69hPjV2/kuuvl+EfkNkQzHsUkghDgy4tIhC+qdsBx30h2dA0jt/raDVyt?=
 =?us-ascii?Q?2fD+nXu077PWZrs18E4pOx6wmB789keULgA3A1giO1J15Hwwo830jmAnm7rg?=
 =?us-ascii?Q?NT5G2AXN3VFiKXKRBw+KGmonnGPn48Qyi2UlO1gSPs1shsbpPiZRSGhqRn4W?=
 =?us-ascii?Q?P/xf0WPw9DwIX/EyiBg0s22sDNaguhUQ98jBxqWL5kuhNFN609EkjmE9ycQL?=
 =?us-ascii?Q?S+NhWIqiDZxkJMpJD3NGRtdVc4ugC2aqs7Irx7YKOslx5a0znNjYA92SvcZU?=
 =?us-ascii?Q?gg3ZoZyk2YSdDUEPAdzU8VrmfMwoLVogNJVP+zlvpNJ/RR9QO5qPWCwzQxQY?=
 =?us-ascii?Q?6FZUjmj5fRPeMxaiKs1TeXgy1ou1+RKnZFMt1Viqj98v+8j9dbmm/gGcAdVI?=
 =?us-ascii?Q?Htuip6eYAluUF0PJ9hAq7/O03MibB/M6xdGNG1HvpiLymgcf1jzxFwMRqPiw?=
 =?us-ascii?Q?+FDZfLjqSb0Pkx7AMj5TeV/TAOA7xzu8YFxKC33h?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 38b2e3f9-ec5a-4db1-6ec3-08dc6867e50f
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Apr 2024 16:17:41.5163
 (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: MoWEjEKl/ezyZOnhpSDkBT3RG8bz+5mbO2148W3C8bIUQoPBoxWj7bnMm207+ZXM9wUT4I9JrikQq3nOPJT1Mw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB8167
X-OriginatorOrg: intel.com

Dan Carpenter wrote:
> On Mon, Apr 29, 2024 at 09:31:53AM +0800, Li Zhijian wrote:
> > construct_region() could return a PTR_ERR() which cannot be derefernced.
> > Moving the dereference behind the error checking to make sure the
> > pointer is valid.
> > 
> 
> No, this patch is unnecessary.
> 
> drivers/cxl/core/region.c
>   3080          /*
>   3081           * Ensure that if multiple threads race to construct_region() for @hpa
>   3082           * one does the construction and the others add to that.
>   3083           */
>   3084          mutex_lock(&cxlrd->range_lock);
>   3085          region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
>   3086                                         match_region_by_range);
>   3087          if (!region_dev) {
>   3088                  cxlr = construct_region(cxlrd, cxled);
>   3089                  region_dev = &cxlr->dev;
>                                      ^^^^^^^^^^^
> This is not a dereference, it's just pointer math.  In in this case it's
> the same as saying:
> 
> 		region_dev = (void *)cxlr;

Ah...  OK I guess we can ignore the change.  Still odd to my eyes though.

Ira

> 
>   3090          } else
>   3091                  cxlr = to_cxl_region(region_dev);
>   3092          mutex_unlock(&cxlrd->range_lock);
>   3093  
>   3094          rc = PTR_ERR_OR_ZERO(cxlr);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This check means that if cxlr is an error pointer then we will clean up
> and return an error.
> 
> regards,
> dan carpenter
> 
>   3095          if (rc)
>   3096                  goto out;
>   3097  
>   3098          attach_target(cxlr, cxled, -1, TASK_UNINTERRUPTIBLE);
>   3099  
>   3100          down_read(&cxl_region_rwsem);
>   3101          p = &cxlr->params;
>   3102          attach = p->state == CXL_CONFIG_COMMIT;
>   3103          up_read(&cxl_region_rwsem);
>   3104  
>   3105          if (attach) {
>   3106                  /*
>   3107                   * If device_attach() fails the range may still be active via
>   3108                   * the platform-firmware memory map, otherwise the driver for
>   3109                   * regions is local to this file, so driver matching can't fail.
>   3110                   */
>   3111                  if (device_attach(&cxlr->dev) < 0)
>   3112                          dev_err(&cxlr->dev, "failed to enable, range: %pr\n",
>   3113                                  p->res);
>   3114          }
>   3115  
>   3116          put_device(region_dev);
>   3117  out:
>   3118          put_device(cxlrd_dev);
>   3119          return rc;
>   3120  }
> 
>