| Message ID | 20240429013154.368118-2-lizhijian@fujitsu.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] cxl/region: Fix potential invalid pointer dereference | expand |
On 29/04/2024 09:31, Li Zhijian wrote: >> mutex_lock(&cxlrd->range_lock); >> region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa, >> match_region_by_range); >> if (!region_dev) >> cxlr = construct_region(cxlrd, cxled); >> else >> cxlr = to_cxl_region(region_dev); >> mutex_unlock(&cxlrd->range_lock); >> >> rc = PTR_ERR_OR_ZERO(cxlr); Think again, PTR_ERR_OR_ZERO() here should be IS_ERR_OR_NULL()? PTR_ERR_OR_ZERO() returns 0 if cxlr is NULL, but the cxlr will be dereferenced later. >> if (rc) >> goto out; >> >> if (!region_dev) >> region_dev = &cxlr->dev; > > When to_cxl_region(region_dev) fails, put_device(region_dev) should be > called to decrease the reference count added by device_find_child(). > > Simply put_device(region_dev) if region_dev is valid in the error path. > > Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery") > Signed-off-by: Li Zhijian <lizhijian@fujitsu.com> > --- > drivers/cxl/core/region.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 3c80aa263a65..75390865382f 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -3117,8 +3117,9 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled) > p->res); > } > > - put_device(region_dev); > out: > + if (region_dev) > + put_device(region_dev); > put_device(cxlrd_dev); > return rc; > }
…
> Simply put_device(region_dev) if region_dev is valid in the error path.
Please improve the change description with a corresponding imperative wording.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc5#n94
Regards,
Markus
On 29/04/2024 16:00, Markus Elfring wrote: > … >> Simply put_device(region_dev) if region_dev is valid in the error path. > > Please improve the change description with a corresponding imperative wording. Yeah, I always overlook this point. thank you. Thanks Zhijian > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc5#n94 > > Regards, > Markus
TBH, even though this patch seems correct, there are still a few
to_cxl_region() callers where they dereference the return value directly
without checking if it's NULL.
So I'm fine to drop this one because to_cxl_region() will trigger a WARN before
returning NULL.
static struct cxl_region *to_cxl_region(struct device *dev)
{
if (dev_WARN_ONCE(dev, dev->type != &cxl_region_type,
"not a cxl_region device\n"))
return NULL;
return container_of(dev, struct cxl_region, dev);
}
Let me know your thoughts.
Thanks
Zhijian
On 29/04/2024 09:31, Li Zhijian wrote:
>> mutex_lock(&cxlrd->range_lock);
>> region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
>> match_region_by_range);
>> if (!region_dev)
>> cxlr = construct_region(cxlrd, cxled);
>> else
>> cxlr = to_cxl_region(region_dev);
>> mutex_unlock(&cxlrd->range_lock);
>>
>> rc = PTR_ERR_OR_ZERO(cxlr);
>> if (rc)
>> goto out;
>>
>> if (!region_dev)
>> region_dev = &cxlr->dev;
>
> When to_cxl_region(region_dev) fails, put_device(region_dev) should be
> called to decrease the reference count added by device_find_child().
>
> Simply put_device(region_dev) if region_dev is valid in the error path.
>
> Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> drivers/cxl/core/region.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 3c80aa263a65..75390865382f 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3117,8 +3117,9 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
> p->res);
> }
>
> - put_device(region_dev);
> out:
> + if (region_dev)
> + put_device(region_dev);
> put_device(cxlrd_dev);
> return rc;
> }
On Mon, Apr 29, 2024 at 08:26:19AM +0000, Zhijian Li (Fujitsu) wrote: > > > On 29/04/2024 16:00, Markus Elfring wrote: > > … > >> Simply put_device(region_dev) if region_dev is valid in the error path. > > > > Please improve the change description with a corresponding imperative wording. > > Yeah, I always overlook this point. thank you. > Greg has a bot that responds to Markus's reviews for USB. https://lore.kernel.org/all/2024042547-shimmy-guileless-c7f2@gregkh/ regards, dan carpenter
On 29/04/2024 18:00, Dan Carpenter wrote: > On Mon, Apr 29, 2024 at 08:26:19AM +0000, Zhijian Li (Fujitsu) wrote: >> >> >> On 29/04/2024 16:00, Markus Elfring wrote: >>> … >>>> Simply put_device(region_dev) if region_dev is valid in the error path. >>> >>> Please improve the change description with a corresponding imperative wording. >> >> Yeah, I always overlook this point. thank you. >> > > Greg has a bot that responds to Markus's reviews for USB.> > https://lore.kernel.org/all/2024042547-shimmy-guileless-c7f2@gregkh/ Wow!! Thanks for your reminder. > > regards, > dan carpenter >
On Mon, Apr 29, 2024 at 09:31:54AM +0800, Li Zhijian wrote: > > mutex_lock(&cxlrd->range_lock); > > region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa, > > match_region_by_range); > > if (!region_dev) > > cxlr = construct_region(cxlrd, cxled); > > else > > cxlr = to_cxl_region(region_dev); > > mutex_unlock(&cxlrd->range_lock); > > > > rc = PTR_ERR_OR_ZERO(cxlr); > > if (rc) > > goto out; > > > > if (!region_dev) > > region_dev = &cxlr->dev; > > When to_cxl_region(region_dev) fails, ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ to_cxl_region() will return NULL if "region_dev" is not a region device. 2215 static struct cxl_region *to_cxl_region(struct device *dev) 2216 { 2217 if (dev_WARN_ONCE(dev, dev->type != &cxl_region_type, 2218 "not a cxl_region device\n")) 2219 return NULL; 2220 2221 return container_of(dev, struct cxl_region, dev); 2222 } It won't fail. If it does fail, we're already in bad shape and it's not worth worrying about resource leaks at that point. regards, dan carpenter
On 29/04/2024 18:17, Dan Carpenter wrote: > On Mon, Apr 29, 2024 at 09:31:54AM +0800, Li Zhijian wrote: >>> mutex_lock(&cxlrd->range_lock); >>> region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa, >>> match_region_by_range); >>> if (!region_dev) >>> cxlr = construct_region(cxlrd, cxled); >>> else >>> cxlr = to_cxl_region(region_dev); >>> mutex_unlock(&cxlrd->range_lock); >>> >>> rc = PTR_ERR_OR_ZERO(cxlr); >>> if (rc) >>> goto out; >>> >>> if (!region_dev) >>> region_dev = &cxlr->dev; >> >> When to_cxl_region(region_dev) fails, > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > to_cxl_region() will return NULL if "region_dev" is not a region device. > > 2215 static struct cxl_region *to_cxl_region(struct device *dev) > 2216 { > 2217 if (dev_WARN_ONCE(dev, dev->type != &cxl_region_type, > 2218 "not a cxl_region device\n")) > 2219 return NULL; > 2220 > 2221 return container_of(dev, struct cxl_region, dev); > 2222 } > > It won't fail. > > If it does fail, we're already in bad shape and it's not worth worrying > about resource leaks at that point. > Sounds good to me, Thanks Zhijian > regards, > dan carpenter > >
Btw, I assume you're doing some kind of static analysis? Looking for missing NULL checks is very tricky and I haven't found a way to do it well except for looking at failed allocations. Allocations have to be checked. There are so many other functions where we leave off the NULL check because the caller knows it can't fail. regards, dan carpenter
Li Zhijian wrote: > > mutex_lock(&cxlrd->range_lock); > > region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa, > > match_region_by_range); > > if (!region_dev) > > cxlr = construct_region(cxlrd, cxled); > > else > > cxlr = to_cxl_region(region_dev); > > mutex_unlock(&cxlrd->range_lock); > > > > rc = PTR_ERR_OR_ZERO(cxlr); > > if (rc) > > goto out; > > > > if (!region_dev) > > region_dev = &cxlr->dev; This diff hunk in the commit message is very odd. I would drop it. We know this builds on patch 1 where you made the above change. > > When to_cxl_region(region_dev) fails, put_device(region_dev) should be > called to decrease the reference count added by device_find_child(). I __think__ this is what Dan was pointing out but I'm not sure. I wanted to point out that to_cxl_region() can't fail because the device_find_child() is checking that the device is a region device. Dan, is that what you were saying when you mentioned there were more serious issues if to_cxl_region() were to fail? Ira > > Simply put_device(region_dev) if region_dev is valid in the error path. > > Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery") > Signed-off-by: Li Zhijian <lizhijian@fujitsu.com> > --- > drivers/cxl/core/region.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 3c80aa263a65..75390865382f 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -3117,8 +3117,9 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled) > p->res); > } > > - put_device(region_dev); > out: > + if (region_dev) > + put_device(region_dev); > put_device(cxlrd_dev); > return rc; > } > -- > 2.29.2 >
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 3c80aa263a65..75390865382f 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -3117,8 +3117,9 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled) p->res); } - put_device(region_dev); out: + if (region_dev) + put_device(region_dev); put_device(cxlrd_dev); return rc; }