diff mbox series

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

Message ID 6f3f15b368b1d2708f93f00325e009747425cef0.1741047738.git.alison.schofield@intel.com (mailing list archive)
State Superseded
Headers show
Series Address Coverity Scan Defects | expand

Commit Message

Alison Schofield March 4, 2025, 12:37 a.m. UTC
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(-)

Comments

Dave Jiang March 5, 2025, 4:38 p.m. UTC | #1
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);
Alison Schofield March 6, 2025, 10:56 p.m. UTC | #2
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);
>
diff mbox series

Patch

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);