linux-kernel - Re: [PATCH 1/2] cxl/region: Fix potential invalid pointer dereference

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Mon, 29 Apr 2024 09:17:38 -0700
From: Ira Weiny <ira.weiny@...el.com>
To: Dan Carpenter <dan.carpenter@...aro.org>, Li Zhijian
	<lizhijian@...itsu.com>
CC: <dave@...olabs.net>, <jonathan.cameron@...wei.com>,
	<dave.jiang@...el.com>, <alison.schofield@...el.com>,
	<vishal.l.verma@...el.com>, <ira.weiny@...el.com>,
	<dan.j.williams@...el.com>, <linux-cxl@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] cxl/region: Fix potential invalid pointer dereference

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  }
> 
>