[ndctl PATCH 0/5] Address Coverity Scan Defects

[ndctl PATCH 0/5] Address Coverity Scan Defects

[alison.schofield]

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

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 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      |  8 +++++---
 ndctl/namespace.c | 37 ++++++++++++++++++++++++++++---------
 2 files changed, 33 insertions(+), 12 deletions(-)


base-commit: d2c55938a0c7308cba07cb23b598df1be93f1d38
-- 
2.37.3

[ndctl PATCH 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 maximimum value that an unsigned long long can hold.

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

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index aa8c23a50385..bb0c2f2e28c7 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -865,9 +865,15 @@ static int validate_namespace_options(struct ndctl_region *region,
 		 * option
 		 */
 		size_align = max(units, size_align) * ways;
-
 		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

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

[Dave Jiang]

On 3/3/25 5:37 PM, alison.schofield@intel.com wrote:
> 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 maximimum 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>

nit below

> ---
>  ndctl/namespace.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/ndctl/namespace.c b/ndctl/namespace.c
> index aa8c23a50385..bb0c2f2e28c7 100644
> --- a/ndctl/namespace.c
> +++ b/ndctl/namespace.c
> @@ -865,9 +865,15 @@ static int validate_namespace_options(struct ndctl_region *region,
>  		 * option
>  		 */
>  		size_align = max(units, size_align) * ways;
> -

stray edit?
>  		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"

[ndctl PATCH 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>
---
 ndctl/namespace.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index bb0c2f2e28c7..5eb9e1e98e11 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -2133,7 +2133,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;
 		}
 	}
 
@@ -2144,11 +2144,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;
@@ -2207,7 +2207,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);
@@ -2286,9 +2286,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
@@ -2303,6 +2300,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

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

[Dave Jiang]

On 3/3/25 5:37 PM, alison.schofield@intel.com wrote:
> 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 bb0c2f2e28c7..5eb9e1e98e11 100644
> --- a/ndctl/namespace.c
> +++ b/ndctl/namespace.c
> @@ -2133,7 +2133,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;
>  		}
>  	}
>  
> @@ -2144,11 +2144,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;
> @@ -2207,7 +2207,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);
> @@ -2286,9 +2286,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
> @@ -2303,6 +2300,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;
>  

[ndctl PATCH 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 move the increment statement to after slot
is evaluated. That keeps the comparison to a u32 as is and avoids
overflow.

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

diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index 889b620355fc..c39c69bfa336 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)
@@ -115,7 +115,6 @@ static struct json_object *dump_label_json(struct ndctl_dimm *dimm,
 		struct json_object *jobj;
 		char uuid[40];
 
-		slot++;
 		jlabel = json_object_new_object();
 		if (!jlabel)
 			break;
@@ -127,8 +126,11 @@ static struct json_object *dump_label_json(struct ndctl_dimm *dimm,
 		if (len < 0)
 			break;
 
-		if (le32_to_cpu(nslabel.slot) != slot)
+		if (le32_to_cpu(nslabel.slot) != slot) {
+			slot++;
 			continue;
+		}
+		slot++;
 
 		uuid_unparse((void *) nslabel.uuid, uuid);
 		jobj = json_object_new_string(uuid);
-- 
2.37.3

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

[Dave Jiang]

On 3/3/25 5:37 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 move the increment statement to after slot
> is evaluated. That keeps the comparison to a u32 as is and avoids
> overflow.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  ndctl/dimm.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/ndctl/dimm.c b/ndctl/dimm.c
> index 889b620355fc..c39c69bfa336 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)
> @@ -115,7 +115,6 @@ static struct json_object *dump_label_json(struct ndctl_dimm *dimm,
>  		struct json_object *jobj;
>  		char uuid[40];
>  
> -		slot++;
>  		jlabel = json_object_new_object();
>  		if (!jlabel)
>  			break;
> @@ -127,8 +126,11 @@ static struct json_object *dump_label_json(struct ndctl_dimm *dimm,
>  		if (len < 0)
>  			break;
>  
> -		if (le32_to_cpu(nslabel.slot) != slot)
> +		if (le32_to_cpu(nslabel.slot) != slot) {
> +			slot++;
>  			continue;
> +		}
> +		slot++;

Wonder if you can just increment the slot in the for() since it's not being used after this. 

>  
>  		uuid_unparse((void *) nslabel.uuid, uuid);
>  		jobj = json_object_new_string(uuid);

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

[Alison Schofield]

On Wed, Mar 05, 2025 at 09:38:31AM -0700, Dave Jiang wrote:
> 
> 
> On 3/3/25 5:37 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 move the increment statement to after slot
> > is evaluated. That keeps the comparison to a u32 as is and avoids
> > overflow.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  ndctl/dimm.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/ndctl/dimm.c b/ndctl/dimm.c
> > index 889b620355fc..c39c69bfa336 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)
> > @@ -115,7 +115,6 @@ static struct json_object *dump_label_json(struct ndctl_dimm *dimm,
> >  		struct json_object *jobj;
> >  		char uuid[40];
> >  
> > -		slot++;
> >  		jlabel = json_object_new_object();
> >  		if (!jlabel)
> >  			break;
> > @@ -127,8 +126,11 @@ static struct json_object *dump_label_json(struct ndctl_dimm *dimm,
> >  		if (len < 0)
> >  			break;
> >  
> > -		if (le32_to_cpu(nslabel.slot) != slot)
> > +		if (le32_to_cpu(nslabel.slot) != slot) {
> > +			slot++;
> >  			continue;
> > +		}
> > +		slot++;
> 
> Wonder if you can just increment the slot in the for() since it's not being used after this. 

Nice - thanks!
Changing to: for (offset = nsindex_size * 2; offset < size;
		  offset += ndctl_dimm_sizeof_namespace_label(dimm), slot++)


> 
> >  
> >  		uuid_unparse((void *) nslabel.uuid, uuid);
> >  		jobj = json_object_new_string(uuid);
> 

[ndctl PATCH 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>
---
 ndctl/namespace.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index 5eb9e1e98e11..40bcf4ca65ac 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -1872,6 +1872,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);
@@ -1913,6 +1917,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

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

[Dave Jiang]

On 3/3/25 5:37 PM, alison.schofield@intel.com wrote:
> 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 5eb9e1e98e11..40bcf4ca65ac 100644
> --- a/ndctl/namespace.c
> +++ b/ndctl/namespace.c
> @@ -1872,6 +1872,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);
> @@ -1913,6 +1917,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

[ndctl PATCH 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>
---
 ndctl/namespace.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index 40bcf4ca65ac..3224c9ff4444 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -2086,7 +2086,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 5/5] ndctl/namespace: protect against under|over-flow w bad param.align

[Dave Jiang]

On 3/3/25 5:37 PM, alison.schofield@intel.com wrote:
> 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 40bcf4ca65ac..3224c9ff4444 100644
> --- a/ndctl/namespace.c
> +++ b/ndctl/namespace.c
> @@ -2086,7 +2086,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);
>