[PATCH ndctl 0/4] cxl: misc coverity and typo fixes

[PATCH ndctl 0/4] cxl: misc coverity and typo fixes

[Vishal Verma]

Fix a few issues reported by Coverity, and a comment typo.

Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

---
Vishal Verma (4):
      ndctl/lib: fix usage of a non NUL-terminated string
      cxl/region: fix a resource leak in to_csv()
      cxl/region: fix an out of bounds access in to_csv()
      cxl/region: fix a comment typo

 ndctl/lib/libndctl.c | 2 ++
 cxl/region.c         | 8 +++++---
 2 files changed, 7 insertions(+), 3 deletions(-)
---
base-commit: 5b57c48998186b894fb94ce099c785d584773402
change-id: 20230110-vv-coverity-fixes-9ea8d3092407

Best regards,
-- 
Vishal Verma <vishal.l.verma@intel.com>

[PATCH ndctl 1/4] ndctl/lib: fix usage of a non NUL-terminated string

[Vishal Verma]

Static analysis reports that in add_region(), a buffer from pread()
won't have NUL-termination. Hence passing it to strtol subsequently can
be wrong. Manually add the termination after pread() to fix this.

Fixes: c64cc150a21e ("ndctl: add support in libndctl to provide deep flush")
Cc: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 ndctl/lib/libndctl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index f32f704..ddbdd9a 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -2750,6 +2750,8 @@ static void *add_region(void *parent, int id, const char *region_base)
 		goto out;
 	}
 
+	/* pread() doesn't add NUL termination */
+	buf[1] = 0;
 	perm = strtol(buf, NULL, 0);
 	if (perm == 0) {
 		close(region->flush_fd);

-- 
2.39.0

[PATCH ndctl 2/4] cxl/region: fix a resource leak in to_csv()

[Vishal Verma]

Static analysis reports there can be a memory leak in to_csv as an exit
path returns from the function before freeing 'csv'. Since this is the
only errpr path exit point after the allocation, just free() before
returning.

Fixes: 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets")
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 cxl/region.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/cxl/region.c b/cxl/region.c
index bb3a10a..9a81113 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -146,8 +146,10 @@ static const char *to_csv(int *count, const char **strings)
 		return NULL;
 	for (i = 0; i < *count; i++) {
 		list = strdup(strings[i]);
-		if (!list)
+		if (!list) {
+			free(csv);
 			return NULL;
+		}
 
 		for (arg = strtok_r(list, which_sep(list), &save); arg;
 		     arg = strtok_r(NULL, which_sep(list), &save)) {

-- 
2.39.0

[PATCH ndctl 3/4] cxl/region: fix an out of bounds access in to_csv()

[Vishal Verma]

Static analysis reports that when 'csv' is allocated for 'len' bytes,
writing to csv[len] results in an out of bounds access. Fix this
truncation operation to instead write the NUL terminator to csv[len -
1], which is the last byte of the memory allocated.

Fixes: 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets")
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 cxl/region.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cxl/region.c b/cxl/region.c
index 9a81113..89be9b5 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -156,7 +156,7 @@ static const char *to_csv(int *count, const char **strings)
 			cursor += snprintf(csv + cursor, len - cursor, "%s%s",
 					   arg, i + 1 < new_count ? "," : "");
 			if (cursor >= len) {
-				csv[len] = 0;
+				csv[len - 1] = 0;
 				break;
 			}
 		}

-- 
2.39.0

[PATCH ndctl 4/4] cxl/region: fix a comment typo

[Vishal Verma]

Fix a typo: s/separted/separated/

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

diff --git a/cxl/region.c b/cxl/region.c
index 89be9b5..efe05aa 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -103,7 +103,7 @@ static const struct option destroy_options[] = {
 
 /*
  * Convert an array of strings that can be a mixture of single items, a
- * command separted list, or a space separated list, into a flattened
+ * command separated list, or a space separated list, into a flattened
  * comma-separated string. That single string can then be used as a
  * filter argument to cxl_filter_walk(), or an ordering constraint for
  * json_object_array_sort()

-- 
2.39.0

Re: [PATCH ndctl 1/4] ndctl/lib: fix usage of a non NUL-terminated string

[Alison Schofield]

On Tue, Jan 10, 2023 at 04:09:14PM -0700, Vishal Verma wrote:
> Static analysis reports that in add_region(), a buffer from pread()
> won't have NUL-termination. Hence passing it to strtol subsequently can
> be wrong. Manually add the termination after pread() to fix this.
> 
> Fixes: c64cc150a21e ("ndctl: add support in libndctl to provide deep flush")
> Cc: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  ndctl/lib/libndctl.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index f32f704..ddbdd9a 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -2750,6 +2750,8 @@ static void *add_region(void *parent, int id, const char *region_base)
>  		goto out;
>  	}
>  
> +	/* pread() doesn't add NUL termination */
> +	buf[1] = 0;
>  	perm = strtol(buf, NULL, 0);
>  	if (perm == 0) {
>  		close(region->flush_fd);
> 

This diff is annoying because it didn't include enough to show the
pread().  Made me open up file ;)

Reviewed-by: Alison Schofield <alison.schofield@intel.com>


> -- 
> 2.39.0
> 

Re: [PATCH ndctl 2/4] cxl/region: fix a resource leak in to_csv()

[Alison Schofield]

On Tue, Jan 10, 2023 at 04:09:15PM -0700, Vishal Verma wrote:
> Static analysis reports there can be a memory leak in to_csv as an exit
> path returns from the function before freeing 'csv'. Since this is the
> only errpr path exit point after the allocation, just free() before
> returning.
> 
> Fixes: 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets")
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

Reviewed-by: Alison Schofield <alison.schofield@intel.com>


> ---
>  cxl/region.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/cxl/region.c b/cxl/region.c
> index bb3a10a..9a81113 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -146,8 +146,10 @@ static const char *to_csv(int *count, const char **strings)
>  		return NULL;
>  	for (i = 0; i < *count; i++) {
>  		list = strdup(strings[i]);
> -		if (!list)
> +		if (!list) {
> +			free(csv);
>  			return NULL;
> +		}
>  
>  		for (arg = strtok_r(list, which_sep(list), &save); arg;
>  		     arg = strtok_r(NULL, which_sep(list), &save)) {
> 
> -- 
> 2.39.0

Re: [PATCH ndctl 3/4] cxl/region: fix an out of bounds access in to_csv()

[Alison Schofield]

On Tue, Jan 10, 2023 at 04:09:16PM -0700, Vishal Verma wrote:
> Static analysis reports that when 'csv' is allocated for 'len' bytes,
> writing to csv[len] results in an out of bounds access. Fix this
> truncation operation to instead write the NUL terminator to csv[len -
> 1], which is the last byte of the memory allocated.
> 
> Fixes: 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets")
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

Reviewed-by: Alison Schofield <alison.schofield@intel.com>


> ---
>  cxl/region.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cxl/region.c b/cxl/region.c
> index 9a81113..89be9b5 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -156,7 +156,7 @@ static const char *to_csv(int *count, const char **strings)
>  			cursor += snprintf(csv + cursor, len - cursor, "%s%s",
>  					   arg, i + 1 < new_count ? "," : "");
>  			if (cursor >= len) {
> -				csv[len] = 0;
> +				csv[len - 1] = 0;
>  				break;
>  			}
>  		}
> 
> -- 
> 2.39.0
> 

Re: [PATCH ndctl 4/4] cxl/region: fix a comment typo

[Alison Schofield]

On Tue, Jan 10, 2023 at 04:09:17PM -0700, Vishal Verma wrote:
> Fix a typo: s/separted/separated/
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

> ---
>  cxl/region.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cxl/region.c b/cxl/region.c
> index 89be9b5..efe05aa 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -103,7 +103,7 @@ static const struct option destroy_options[] = {
>  
>  /*
>   * Convert an array of strings that can be a mixture of single items, a
> - * command separted list, or a space separated list, into a flattened
> + * command separated list, or a space separated list, into a flattened
>   * comma-separated string. That single string can then be used as a
>   * filter argument to cxl_filter_walk(), or an ordering constraint for
>   * json_object_array_sort()
> 
> -- 
> 2.39.0