[ndctl PATCH v2 0/5] Address Coverity Scan Defects

[ndctl PATCH v2 0/5] Address Coverity Scan Defects

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

Changes in v2:
1/5: Put back deleted whitespace (DaveJ)
     s/maximimum/maximum
3/5: Move slot++ to for loop header (DaveJ)
     Update commit log to match

The NDCTL project routinely addresses any and all coverity defects
before each release. This new set of reports suggests enhancements
to the scan capabilities since none of this code has been touched
recently. Game on, we'll squash 'em!

Alison Schofield (5):
  ndctl/namespace: avoid integer overflow in namespace validation
  ndctl/namespace: close file descriptor in do_xaction_namespace()
  ndctl/dimm: do not increment a ULLONG_MAX slot value
  ndctl/namespace: protect against overflow handling param.offset
  ndctl/namespace: protect against under|over-flow w bad param.align

 ndctl/dimm.c      |  5 ++---
 ndctl/namespace.c | 36 ++++++++++++++++++++++++++++--------
 2 files changed, 30 insertions(+), 11 deletions(-)


base-commit: d2c55938a0c7308cba07cb23b598df1be93f1d38
-- 
2.37.3

[ndctl PATCH v2 1/5] ndctl/namespace: avoid integer overflow in namespace validation

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

A coverity scan highlighted an integer overflow issue when testing
if the size and align parameters make sense together.

Before performing the multiplication, check that the result will not
exceed the maximum value that an unsigned long long can hold.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
---
 ndctl/namespace.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index aa8c23a50385..372fc3747c88 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -868,6 +868,13 @@ static int validate_namespace_options(struct ndctl_region *region,
 
 		p->size /= size_align;
 		p->size++;
+
+		if (p->size > ULLONG_MAX / size_align) {
+			err("size overflow: %llu * %llu exceeds ULLONG_MAX\n",
+			    p->size, size_align);
+			return -EINVAL;
+		}
+
 		p->size *= size_align;
 		p->size /= units;
 		err("'--size=' must align to interleave-width: %d and alignment: %ld\n"
-- 
2.37.3

[ndctl PATCH v2 2/5] ndctl/namespace: close file descriptor in do_xaction_namespace()

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

A coverity scan highlighted a resource leak caused by not freeing
the open file descriptor upon exit of do_xaction_namespace().

Move the fclose() to a 'goto out_close' and route all returns through
that path.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
---
 ndctl/namespace.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index 372fc3747c88..6c86eadcad69 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -2134,7 +2134,7 @@ static int do_xaction_namespace(const char *namespace,
 				util_display_json_array(ri_ctx.f_out, ri_ctx.jblocks, 0);
 			if (rc >= 0)
 				(*processed)++;
-			return rc;
+			goto out_close;
 		}
 	}
 
@@ -2145,11 +2145,11 @@ static int do_xaction_namespace(const char *namespace,
 		rc = file_write_infoblock(param.outfile);
 		if (rc >= 0)
 			(*processed)++;
-		return rc;
+		goto out_close;
 	}
 
 	if (!namespace && action != ACTION_CREATE)
-		return rc;
+		goto out_close;
 
 	if (namespace && (strcmp(namespace, "all") == 0))
 		rc = 0;
@@ -2208,7 +2208,7 @@ static int do_xaction_namespace(const char *namespace,
 						saved_rc = rc;
 						continue;
 				}
-				return rc;
+				goto out_close;
 			}
 			ndctl_namespace_foreach_safe(region, ndns, _n) {
 				ndns_name = ndctl_namespace_get_devname(ndns);
@@ -2287,9 +2287,6 @@ static int do_xaction_namespace(const char *namespace,
 	if (ri_ctx.jblocks)
 		util_display_json_array(ri_ctx.f_out, ri_ctx.jblocks, 0);
 
-	if (ri_ctx.f_out && ri_ctx.f_out != stdout)
-		fclose(ri_ctx.f_out);
-
 	if (action == ACTION_CREATE && rc == -EAGAIN) {
 		/*
 		 * Namespace creation searched through all candidate
@@ -2304,6 +2301,10 @@ static int do_xaction_namespace(const char *namespace,
 		else
 			rc = -ENOSPC;
 	}
+
+out_close:
+	if (ri_ctx.f_out && ri_ctx.f_out != stdout)
+		fclose(ri_ctx.f_out);
 	if (saved_rc)
 		rc = saved_rc;
 
-- 
2.37.3

[ndctl PATCH v2 3/5] ndctl/dimm: do not increment a ULLONG_MAX slot value

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

A coverity scan higlighted an overflow issue when the slot variable,
an unsigned integer that is initialized to -1, is incremented and
overflows.

Initialize slot to 0 and increment slot in the for loop header. That
keeps the comparison to a u32 as is and avoids overflow.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 ndctl/dimm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index 889b620355fc..aaa0abfa046c 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -97,7 +97,7 @@ static struct json_object *dump_label_json(struct ndctl_dimm *dimm,
 	struct json_object *jlabel = NULL;
 	struct namespace_label nslabel;
 	unsigned int nsindex_size;
-	unsigned int slot = -1;
+	unsigned int slot = 0;
 	ssize_t offset;
 
 	if (!jarray)
@@ -108,14 +108,13 @@ static struct json_object *dump_label_json(struct ndctl_dimm *dimm,
 		return NULL;
 
 	for (offset = nsindex_size * 2; offset < size;
-			offset += ndctl_dimm_sizeof_namespace_label(dimm)) {
+	     offset += ndctl_dimm_sizeof_namespace_label(dimm), slot++) {
 		ssize_t len = min_t(ssize_t,
 				ndctl_dimm_sizeof_namespace_label(dimm),
 				size - offset);
 		struct json_object *jobj;
 		char uuid[40];
 
-		slot++;
 		jlabel = json_object_new_object();
 		if (!jlabel)
 			break;
-- 
2.37.3

[ndctl PATCH v2 4/5] ndctl/namespace: protect against overflow handling param.offset

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

A param.offset is parsed using parse_size64() but the result is
not checked for the error return ULLONG_MAX. If ULLONG_MAX is
returned, follow-on calculations will lead to overflow.

Add check for ULLONG_MAX upon return from parse_size64.
Add check for overflow in subsequent PFN_MODE offset calculation.

This issue was reported in a coverity scan.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
---
 ndctl/namespace.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index 6c86eadcad69..2cee1c4c1451 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -1873,6 +1873,10 @@ static int write_pfn_sb(int fd, unsigned long long size, const char *sig,
 	int rc;
 
 	start = parse_size64(param.offset);
+	if (start == ULLONG_MAX) {
+		err("failed to parse offset option '%s'\n", param.offset);
+		return -EINVAL;
+	}
 	npfns = PHYS_PFN(size - SZ_8K);
 	pfn_align = parse_size64(param.align);
 	align = max(pfn_align, SUBSECTION_SIZE);
@@ -1914,6 +1918,10 @@ static int write_pfn_sb(int fd, unsigned long long size, const char *sig,
 		 * struct page size. But we also want to make sure we notice
 		 * when we end up adding new elements to struct page.
 		 */
+		if (start > ULLONG_MAX - (SZ_8K + MAX_STRUCT_PAGE_SIZE * npfns)) {
+			error("integer overflow in offset calculation\n");
+			return -EINVAL;
+		}
 		offset = ALIGN(start + SZ_8K + MAX_STRUCT_PAGE_SIZE * npfns, align)
 			- start;
 	} else
-- 
2.37.3

[ndctl PATCH v2 5/5] ndctl/namespace: protect against under|over-flow w bad param.align

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

A coverity scan highlighted an integer underflow when param.align
is 0, and an integer overflow when the parsing of param.align fails
and returns ULLONG_MAX.

Add explicit checks for both values.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
---
 ndctl/namespace.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index 2cee1c4c1451..e443130a5a93 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -2087,7 +2087,11 @@ static int namespace_rw_infoblock(struct ndctl_namespace *ndns,
 			unsigned long long size = parse_size64(param.size);
 			align = parse_size64(param.align);
 
-			if (align < ULLONG_MAX && !IS_ALIGNED(size, align)) {
+			if (align == 0 || align == ULLONG_MAX) {
+				error("invalid alignment:%s\n", param.align);
+				rc = -EINVAL;
+			}
+			if (!IS_ALIGNED(size, align)) {
 				error("--size=%s not aligned to %s\n", param.size,
 					param.align);
 
-- 
2.37.3

Re: [ndctl PATCH v2 3/5] ndctl/dimm: do not increment a ULLONG_MAX slot value

[Dave Jiang]

On 3/6/25 4:50 PM, alison.schofield@intel.com wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> A coverity scan higlighted an overflow issue when the slot variable,
> an unsigned integer that is initialized to -1, is incremented and
> overflows.
> 
> Initialize slot to 0 and increment slot in the for loop header. That
> keeps the comparison to a u32 as is and avoids overflow.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  ndctl/dimm.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/ndctl/dimm.c b/ndctl/dimm.c
> index 889b620355fc..aaa0abfa046c 100644
> --- a/ndctl/dimm.c
> +++ b/ndctl/dimm.c
> @@ -97,7 +97,7 @@ static struct json_object *dump_label_json(struct ndctl_dimm *dimm,
>  	struct json_object *jlabel = NULL;
>  	struct namespace_label nslabel;
>  	unsigned int nsindex_size;
> -	unsigned int slot = -1;
> +	unsigned int slot = 0;
>  	ssize_t offset;
>  
>  	if (!jarray)
> @@ -108,14 +108,13 @@ static struct json_object *dump_label_json(struct ndctl_dimm *dimm,
>  		return NULL;
>  
>  	for (offset = nsindex_size * 2; offset < size;
> -			offset += ndctl_dimm_sizeof_namespace_label(dimm)) {
> +	     offset += ndctl_dimm_sizeof_namespace_label(dimm), slot++) {
>  		ssize_t len = min_t(ssize_t,
>  				ndctl_dimm_sizeof_namespace_label(dimm),
>  				size - offset);
>  		struct json_object *jobj;
>  		char uuid[40];
>  
> -		slot++;
>  		jlabel = json_object_new_object();
>  		if (!jlabel)
>  			break;