[ndctl PATCH v2 0/3] cxl: static analysis fixes

[ndctl PATCH v2 0/3] cxl: static analysis fixes

[Vishal Verma]

Changes since v1[1]:
- Fix the decoder filter check in patch 1.
- Fix a missed free(path) in patch 2.

[1]: https://lore.kernel.org/linux-cxl/20220823072106.398076-1-vishal.l.verma@intel.com

---

Fix a small handful of issues reported by scan.coverity.com for the
recent region management additions.

Vishal Verma (3):
  cxl/region: fix a dereferecnce after NULL check
  libcxl: fox a resource leak and a forward NULL check
  cxl/filter: Fix an uninitialized pointer dereference

 cxl/lib/libcxl.c | 4 +++-
 cxl/filter.c     | 2 +-
 cxl/region.c     | 5 ++---
 3 files changed, 6 insertions(+), 5 deletions(-)


base-commit: 9a993ce24fdd5de45774b65211570dd514cdf61d
-- 
2.37.2

[ndctl PATCH v2 1/3] cxl/region: fix a dereferecnce after NULL check

[Vishal Verma]

A NULL check in region_action() implies that 'decoder' might be NULL, but
later we dereference it during cxl_decoder_foreach(). The NULL check is
valid because it was the filter result being checked, however, while
doing this, the original 'decoder' variable was being clobbered.

Check the filter results independently of the original decoder variable.

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 cxl/region.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/cxl/region.c b/cxl/region.c
index a30313c..334fcc2 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -686,9 +686,8 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
 			continue;
 
 		cxl_decoder_foreach (port, decoder) {
-			decoder = util_cxl_decoder_filter(decoder,
-							  param.root_decoder);
-			if (!decoder)
+			if (!util_cxl_decoder_filter(decoder,
+						     param.root_decoder))
 				continue;
 			rc = decoder_region_action(p, decoder, action, count);
 			if (rc)
-- 
2.37.2

RE: [ndctl PATCH v2 1/3] cxl/region: fix a dereferecnce after NULL check

[Dan Williams]

Vishal Verma wrote:
> A NULL check in region_action() implies that 'decoder' might be NULL, but
> later we dereference it during cxl_decoder_foreach(). The NULL check is
> valid because it was the filter result being checked, however, while
> doing this, the original 'decoder' variable was being clobbered.
> 
> Check the filter results independently of the original decoder variable.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  cxl/region.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/cxl/region.c b/cxl/region.c
> index a30313c..334fcc2 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -686,9 +686,8 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
>  			continue;
>  
>  		cxl_decoder_foreach (port, decoder) {
> -			decoder = util_cxl_decoder_filter(decoder,
> -							  param.root_decoder);
> -			if (!decoder)
> +			if (!util_cxl_decoder_filter(decoder,
> +						     param.root_decoder))
>  				continue;

Looks good.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Re: [ndctl PATCH v2 1/3] cxl/region: fix a dereferecnce after NULL check

[Dave Jiang]

On 8/23/2022 12:45 AM, Vishal Verma wrote:
> A NULL check in region_action() implies that 'decoder' might be NULL, but
> later we dereference it during cxl_decoder_foreach(). The NULL check is
> valid because it was the filter result being checked, however, while
> doing this, the original 'decoder' variable was being clobbered.
>
> Check the filter results independently of the original decoder variable.
>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   cxl/region.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/cxl/region.c b/cxl/region.c
> index a30313c..334fcc2 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -686,9 +686,8 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
>   			continue;
>   
>   		cxl_decoder_foreach (port, decoder) {
> -			decoder = util_cxl_decoder_filter(decoder,
> -							  param.root_decoder);
> -			if (!decoder)
> +			if (!util_cxl_decoder_filter(decoder,
> +						     param.root_decoder))
>   				continue;
>   			rc = decoder_region_action(p, decoder, action, count);
>   			if (rc)

Re: [ndctl PATCH v2 1/3] cxl/region: fix a dereferecnce after NULL check

[Jonathan Cameron]

On Tue, 23 Aug 2022 01:45:25 -0600
Vishal Verma <vishal.l.verma@intel.com> wrote:

> A NULL check in region_action() implies that 'decoder' might be NULL, but
> later we dereference it during cxl_decoder_foreach(). The NULL check is
> valid because it was the filter result being checked, however, while
> doing this, the original 'decoder' variable was being clobbered.
> 
> Check the filter results independently of the original decoder variable.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

Typo in patch title.

> ---
>  cxl/region.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/cxl/region.c b/cxl/region.c
> index a30313c..334fcc2 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -686,9 +686,8 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
>  			continue;
>  
>  		cxl_decoder_foreach (port, decoder) {
> -			decoder = util_cxl_decoder_filter(decoder,
> -							  param.root_decoder);
> -			if (!decoder)
> +			if (!util_cxl_decoder_filter(decoder,
> +						     param.root_decoder))
>  				continue;
>  			rc = decoder_region_action(p, decoder, action, count);
>  			if (rc)

[ndctl PATCH v2 2/3] libcxl: fox a resource leak and a forward NULL check

[Vishal Verma]

Static analysis reports a couple of issues in add_cxl_region(). Firstly,
'path' wasn't freed in the success case, only in the error case.
Secondly, the error handling after 'calloc()'ing the region object
erroneously jumped to the error path which tried to free the region object.

Add anew error label to just free 'path' and return for this exit case.

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 cxl/lib/libcxl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index 021d59f..e8c5d44 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -482,7 +482,7 @@ static void *add_cxl_region(void *parent, int id, const char *cxlregion_base)
 
 	region = calloc(1, sizeof(*region));
 	if (!region)
-		goto err;
+		goto err_path;
 
 	region->id = id;
 	region->ctx = ctx;
@@ -551,11 +551,13 @@ static void *add_cxl_region(void *parent, int id, const char *cxlregion_base)
 
 	list_add_sorted(&decoder->regions, region, list, region_start_cmp);
 
+	free(path);
 	return region;
 err:
 	free(region->dev_path);
 	free(region->dev_buf);
 	free(region);
+err_path:
 	free(path);
 	return NULL;
 }
-- 
2.37.2

RE: [ndctl PATCH v2 2/3] libcxl: fox a resource leak and a forward NULL check

[Dan Williams]

Vishal Verma wrote:
> Static analysis reports a couple of issues in add_cxl_region(). Firstly,
> 'path' wasn't freed in the success case, only in the error case.
> Secondly, the error handling after 'calloc()'ing the region object
> erroneously jumped to the error path which tried to free the region object.
> 
> Add anew error label to just free 'path' and return for this exit case.

s/anew/a new/

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Re: [ndctl PATCH v2 2/3] libcxl: fox a resource leak and a forward NULL check

[Dave Jiang]

On 8/23/2022 12:45 AM, Vishal Verma wrote:
> Static analysis reports a couple of issues in add_cxl_region(). Firstly,
> 'path' wasn't freed in the success case, only in the error case.
> Secondly, the error handling after 'calloc()'ing the region object
> erroneously jumped to the error path which tried to free the region object.
>
> Add anew error label to just free 'path' and return for this exit case.
>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   cxl/lib/libcxl.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index 021d59f..e8c5d44 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -482,7 +482,7 @@ static void *add_cxl_region(void *parent, int id, const char *cxlregion_base)
>   
>   	region = calloc(1, sizeof(*region));
>   	if (!region)
> -		goto err;
> +		goto err_path;
>   
>   	region->id = id;
>   	region->ctx = ctx;
> @@ -551,11 +551,13 @@ static void *add_cxl_region(void *parent, int id, const char *cxlregion_base)
>   
>   	list_add_sorted(&decoder->regions, region, list, region_start_cmp);
>   
> +	free(path);
>   	return region;
>   err:
>   	free(region->dev_path);
>   	free(region->dev_buf);
>   	free(region);
> +err_path:
>   	free(path);
>   	return NULL;
>   }

[ndctl PATCH v2 3/3] cxl/filter: Fix an uninitialized pointer dereference

[Vishal Verma]

Static analysis points out that there was a chance that 'jdecoder' could
be used while uninitialized in walk_decoders(). Initialize it to NULL to
avoid this.

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 cxl/filter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cxl/filter.c b/cxl/filter.c
index 9a3de8c..56c6599 100644
--- a/cxl/filter.c
+++ b/cxl/filter.c
@@ -796,7 +796,7 @@ static void walk_decoders(struct cxl_port *port, struct cxl_filter_params *p,
 	cxl_decoder_foreach(port, decoder) {
 		const char *devname = cxl_decoder_get_devname(decoder);
 		struct json_object *jchildregions = NULL;
-		struct json_object *jdecoder;
+		struct json_object *jdecoder = NULL;
 
 		if (!p->decoders)
 			goto walk_children;
-- 
2.37.2

RE: [ndctl PATCH v2 3/3] cxl/filter: Fix an uninitialized pointer dereference

[Dan Williams]

Vishal Verma wrote:
> Static analysis points out that there was a chance that 'jdecoder' could
> be used while uninitialized in walk_decoders(). Initialize it to NULL to
> avoid this.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  cxl/filter.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cxl/filter.c b/cxl/filter.c
> index 9a3de8c..56c6599 100644
> --- a/cxl/filter.c
> +++ b/cxl/filter.c
> @@ -796,7 +796,7 @@ static void walk_decoders(struct cxl_port *port, struct cxl_filter_params *p,
>  	cxl_decoder_foreach(port, decoder) {
>  		const char *devname = cxl_decoder_get_devname(decoder);
>  		struct json_object *jchildregions = NULL;
> -		struct json_object *jdecoder;
> +		struct json_object *jdecoder = NULL;

Looks good,

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Re: [ndctl PATCH v2 3/3] cxl/filter: Fix an uninitialized pointer dereference

[Dave Jiang]

On 8/23/2022 12:45 AM, Vishal Verma wrote:
> Static analysis points out that there was a chance that 'jdecoder' could
> be used while uninitialized in walk_decoders(). Initialize it to NULL to
> avoid this.
>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   cxl/filter.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/cxl/filter.c b/cxl/filter.c
> index 9a3de8c..56c6599 100644
> --- a/cxl/filter.c
> +++ b/cxl/filter.c
> @@ -796,7 +796,7 @@ static void walk_decoders(struct cxl_port *port, struct cxl_filter_params *p,
>   	cxl_decoder_foreach(port, decoder) {
>   		const char *devname = cxl_decoder_get_devname(decoder);
>   		struct json_object *jchildregions = NULL;
> -		struct json_object *jdecoder;
> +		struct json_object *jdecoder = NULL;
>   
>   		if (!p->decoders)
>   			goto walk_children;

Re: [ndctl PATCH v2 0/3] cxl: static analysis fixes

[Jonathan Cameron]

On Tue, 23 Aug 2022 01:45:24 -0600
Vishal Verma <vishal.l.verma@intel.com> wrote:

> Changes since v1[1]:
> - Fix the decoder filter check in patch 1.
> - Fix a missed free(path) in patch 2.
> 
> [1]: https://lore.kernel.org/linux-cxl/20220823072106.398076-1-vishal.l.verma@intel.com
FWIW: Series looks fine to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> ---
> 
> Fix a small handful of issues reported by scan.coverity.com for the
> recent region management additions.
> 
> Vishal Verma (3):
>   cxl/region: fix a dereferecnce after NULL check
>   libcxl: fox a resource leak and a forward NULL check
>   cxl/filter: Fix an uninitialized pointer dereference
> 
>  cxl/lib/libcxl.c | 4 +++-
>  cxl/filter.c     | 2 +-
>  cxl/region.c     | 5 ++---
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> 
> base-commit: 9a993ce24fdd5de45774b65211570dd514cdf61d