diff mbox series

[1/2] cxl/region: Fix potential invalid pointer dereference

Message ID 20240429013154.368118-1-lizhijian@fujitsu.com
State Rejected
Headers show
Series [1/2] cxl/region: Fix potential invalid pointer dereference | expand

Commit Message

Zhijian Li (Fujitsu) April 29, 2024, 1:31 a.m. UTC
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.

Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 drivers/cxl/core/region.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Markus Elfring April 29, 2024, 7:50 a.m. UTC | #1
I would usually expect a corresponding cover letter for patch series.


> construct_region() could return a PTR_ERR() which cannot be derefernced.

I hope that a typo will be avoided in the last word of this sentence.


> Moving the dereference behind the error checking to make sure the
> pointer is valid.

Please choose an imperative wording for an improved change description.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc5#n94


…
> +++ b/drivers/cxl/core/region.c
> @@ -3086,10 +3086,9 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
>  	mutex_lock(&cxlrd->range_lock);
>  	region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
>  				       match_region_by_range);
> -	if (!region_dev) {
> +	if (!region_dev)
>  		cxlr = construct_region(cxlrd, cxled);
> -		region_dev = &cxlr->dev;
> -	} else
> +	else
>  		cxlr = to_cxl_region(region_dev);
>  	mutex_unlock(&cxlrd->range_lock);

I suggest to simplify such source code by using a conditional operator expression.

Regards,
Markus
Zhijian Li (Fujitsu) April 29, 2024, 8:43 a.m. UTC | #2
On 29/04/2024 15:50, Markus Elfring wrote:
> I would usually expect a corresponding cover letter for patch series.



> 
> 
>> construct_region() could return a PTR_ERR() which cannot be derefernced.
> 
> I hope that a typo will be avoided in the last word of this sentence.

Thanks, hate my fat fingers, I will fix it later.




> 
> 
>> Moving the dereference behind the error checking to make sure the
>> pointer is valid.
> 
> Please choose an imperative wording for an improved change description.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc5#n94
> 
> 
> …
>> +++ b/drivers/cxl/core/region.c
>> @@ -3086,10 +3086,9 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
>>   	mutex_lock(&cxlrd->range_lock);
>>   	region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
>>   				       match_region_by_range);
>> -	if (!region_dev) {
>> +	if (!region_dev)
>>   		cxlr = construct_region(cxlrd, cxled);
>> -		region_dev = &cxlr->dev;
>> -	} else
>> +	else
>>   		cxlr = to_cxl_region(region_dev);
>>   	mutex_unlock(&cxlrd->range_lock);
> 
> I suggest to simplify such source code by using a conditional operator expression.

Thanks for your suggestion. Do you mean something like:
cxlr = region_dev ? to_cxl_region(region_dev) : construct_region(cxlrd, cxled);

If so, I'm open to this option, but the kernel does not always obey this convention.
For example,
static inline int __must_check PTR_ERR_OR_ZERO(__force const void *ptr)
{
         if (IS_ERR(ptr))
                 return PTR_ERR(ptr);
         else
                 return 0;
}

Thanks
Zhijian

> 
> Regards,
> Markus
Markus Elfring April 29, 2024, 8:55 a.m. UTC | #3
>> I suggest to simplify such source code by using a conditional operator expression.
>
> Thanks for your suggestion. Do you mean something like:
> cxlr = region_dev ? to_cxl_region(region_dev) : construct_region(cxlrd, cxled);

Yes.


> If so, I'm open to this option, but the kernel does not always obey this convention.

Involved developers present different coding style preferences.
I hope that further source code parts can become a bit more succinct.

Regards,
Markus
Dan Carpenter April 29, 2024, 10:10 a.m. UTC | #4
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;

  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  }
Zhijian Li (Fujitsu) April 29, 2024, 10:25 a.m. UTC | #5
On 29/04/2024 18:10, 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.

Agree,


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


You are right, a equivalent code could be:
		region_dev = ((char *)cxlr) + offsetof(struct cxl_region, dev);


Thanks


> 
>    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  }
> 
>
Dan Carpenter April 29, 2024, 10:30 a.m. UTC | #6
On Mon, Apr 29, 2024 at 10:25:35AM +0000, Zhijian Li (Fujitsu) wrote:
> >    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;
> 
> 
> You are right, a equivalent code could be:
> 		region_dev = ((char *)cxlr) + offsetof(struct cxl_region, dev);
> 
> 

Correct.  But offsetof() is zero.  It's the same math that to_cxl_region()
does.

regards,
dan carpenter
Ira Weiny April 29, 2024, 4:05 p.m. UTC | #7
Li Zhijian wrote:
> construct_region() could return a PTR_ERR() which cannot be derefernced.
                                                             ^^^^
                                                             dereferenced

> Moving the dereference behind the error checking to make sure the
> pointer is valid.
> 

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>  drivers/cxl/core/region.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 8535718a20f0..3c80aa263a65 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3086,10 +3086,9 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
>  	mutex_lock(&cxlrd->range_lock);
>  	region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
>  				       match_region_by_range);
> -	if (!region_dev) {
> +	if (!region_dev)
>  		cxlr = construct_region(cxlrd, cxled);
> -		region_dev = &cxlr->dev;
> -	} else
> +	else
>  		cxlr = to_cxl_region(region_dev);
>  	mutex_unlock(&cxlrd->range_lock);
>  
> @@ -3097,6 +3096,9 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
>  	if (rc)
>  		goto out;
>  
> +	if (!region_dev)
> +		region_dev = &cxlr->dev;
> +
>  	attach_target(cxlr, cxled, -1, TASK_UNINTERRUPTIBLE);
>  
>  	down_read(&cxl_region_rwsem);
> -- 
> 2.29.2
>
Ira Weiny April 29, 2024, 4:17 p.m. UTC | #8
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  }
> 
>
diff mbox series

Patch

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 8535718a20f0..3c80aa263a65 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3086,10 +3086,9 @@  int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
 	mutex_lock(&cxlrd->range_lock);
 	region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
 				       match_region_by_range);
-	if (!region_dev) {
+	if (!region_dev)
 		cxlr = construct_region(cxlrd, cxled);
-		region_dev = &cxlr->dev;
-	} else
+	else
 		cxlr = to_cxl_region(region_dev);
 	mutex_unlock(&cxlrd->range_lock);
 
@@ -3097,6 +3096,9 @@  int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
 	if (rc)
 		goto out;
 
+	if (!region_dev)
+		region_dev = &cxlr->dev;
+
 	attach_target(cxlr, cxled, -1, TASK_UNINTERRUPTIBLE);
 
 	down_read(&cxl_region_rwsem);