[PATCH] Fix return value from fstat calls

[PATCH] Fix return value from fstat calls

[Nigel Croxon]

To meet requirements of Common Criteria certification vulnerablility
assessment. Static code analysis has been run and found the following
errors:
check_return: Calling "fstat(fd, &dstb)" without checking return value.
This library function may fail and return an error code.

Changes are to add a test to the return value from fstat calls.

Signed-off-by: Nigel Croxon <ncroxon@redhat.com>
---
 Assemble.c    | 21 +++++++++++++++++++--
 Dump.c        | 15 +++++++++++++--
 Grow.c        | 12 ++++++++++--
 config.c      |  5 ++++-
 managemon.c   |  9 ++++++---
 mdadm.h       |  2 +-
 mdstat.c      | 10 ++++++++--
 super-ddf.c   | 11 +++++++++--
 super-intel.c | 11 +++++++++--
 9 files changed, 79 insertions(+), 17 deletions(-)

diff --git a/Assemble.c b/Assemble.c
index 0df46244..cae3224b 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -649,7 +649,14 @@ static int load_devices(struct devs *devices, char *devmap,
 			/* prepare useful information in info structures */
 			struct stat stb2;
 			int err;
-			fstat(mdfd, &stb2);
+
+			if (fstat(mdfd, &stb2) != 0) {
+				pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
+				close(mdfd);
+				free(devices);
+				free(devmap);
+				return -1;
+			}
 
 			if (strcmp(c->update, "uuid") == 0 && !ident->uuid_set)
 				random_uuid((__u8 *)ident->uuid);
@@ -760,7 +767,17 @@ static int load_devices(struct devs *devices, char *devmap,
 			tst->ss->getinfo_super(tst, content, devmap + devcnt * content->array.raid_disks);
 		}
 
-		fstat(dfd, &stb);
+		if (fstat(dfd, &stb) != 0) {
+			pr_err("fstat failed for %s: %s - aborting\n",devname, strerror(errno));
+			close(dfd);
+			close(mdfd);
+			free(devices);
+			free(devmap);
+			tst->ss->free_super(tst);
+			free(tst);
+			*stp = st;
+			return -1;
+		}
 		close(dfd);
 
 		if (c->verbose > 0)
diff --git a/Dump.c b/Dump.c
index 736bcb60..d1a8bb86 100644
--- a/Dump.c
+++ b/Dump.c
@@ -112,7 +112,14 @@ int Dump_metadata(char *dev, char *dir, struct context *c,
 	}
 	if (c->verbose >= 0)
 		printf("%s saved as %s.\n", dev, fname);
-	fstat(fd, &dstb);
+
+	if (fstat(fd, &dstb) != 0) {
+		pr_err("fstat failed from %s: %s\n",fname, strerror(errno));
+		close(fd);
+		close(fl);
+		free(fname);
+		return 1;
+	}
 	close(fd);
 	close(fl);
 	if ((dstb.st_mode & S_IFMT) != S_IFBLK) {
@@ -200,7 +207,11 @@ int Restore_metadata(char *dev, char *dir, struct context *c,
 		char *chosen = NULL;
 		unsigned int chosen_inode = 0;
 
-		fstat(fd, &dstb);
+		if (fstat(fd, &dstb) != 0) {
+			pr_err("fstat failed for %s: %s\n",dev, strerror(errno));
+			close(fd);
+			return 1;
+		}
 
 		while (d && (de = readdir(d)) != NULL) {
 			if (de->d_name[0] == '.')
diff --git a/Grow.c b/Grow.c
index 7506ab46..4c3ec9c1 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1163,9 +1163,17 @@ int reshape_open_backup_file(char *backup_file,
 	 * way this will not notice, but it is better than
 	 * nothing.
 	 */
-	fstat(*fdlist, &stb);
+	if (fstat(*fdlist, &stb) != 0) {
+		pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
+		close(*fdlist);
+		return 0;
+	}
 	dev = stb.st_dev;
-	fstat(fd, &stb);
+	if (fstat(fd, &stb) != 0) {
+		pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
+		close(*fdlist);
+		return 0;
+	}
 	if (stb.st_rdev == dev) {
 		pr_err("backup file must NOT be on the array being reshaped.\n");
 		close(*fdlist);
diff --git a/config.c b/config.c
index 9c725457..ad0e02fe 100644
--- a/config.c
+++ b/config.c
@@ -803,7 +803,10 @@ void conf_file_or_dir(FILE *f)
 	struct dirent *dp;
 	struct fname *list = NULL;
 
-	fstat(fileno(f), &st);
+	if (fstat(fileno(f), &st) != 0) {
+		pr_err("fstat failed: %s\n", strerror(errno));
+		return;
+	}
 	if (S_ISREG(st.st_mode))
 		conf_file(f);
 	else if (!S_ISDIR(st.st_mode))
diff --git a/managemon.c b/managemon.c
index 200cf83e..d943f2cc 100644
--- a/managemon.c
+++ b/managemon.c
@@ -894,6 +894,7 @@ void do_manager(struct supertype *container)
 {
 	struct mdstat_ent *mdstat;
 	sigset_t set;
+	int rtn;
 
 	sigprocmask(SIG_UNBLOCK, NULL, &set);
 	sigdelset(&set, SIGUSR1);
@@ -926,9 +927,11 @@ void do_manager(struct supertype *container)
 		if (sigterm)
 			wakeup_monitor();
 
-		if (update_queue == NULL)
-			mdstat_wait_fd(container->sock, &set);
-		else
+		if (update_queue == NULL) {
+			rtn = mdstat_wait_fd(container->sock, &set);
+			if (rtn)
+				exit(0);
+		} else
 			/* If an update is happening, just wait for signal */
 			pselect(0, NULL, NULL, NULL, NULL, &set);
 	} while(1);
diff --git a/mdadm.h b/mdadm.h
index 8f8841d8..1273029a 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -637,7 +637,7 @@ extern struct mdstat_ent *mdstat_read(int hold, int start);
 extern void mdstat_close(void);
 extern void free_mdstat(struct mdstat_ent *ms);
 extern int mdstat_wait(int seconds);
-extern void mdstat_wait_fd(int fd, const sigset_t *sigmask);
+extern int mdstat_wait_fd(int fd, const sigset_t *sigmask);
 extern int mddev_busy(char *devnm);
 extern struct mdstat_ent *mdstat_by_component(char *name);
 extern struct mdstat_ent *mdstat_by_subdev(char *subdev, char *container);
diff --git a/mdstat.c b/mdstat.c
index 2fd792c5..8664c0ec 100644
--- a/mdstat.c
+++ b/mdstat.c
@@ -336,7 +336,7 @@ int mdstat_wait(int seconds)
 	return select(maxfd + 1, NULL, NULL, &fds, &tm);
 }
 
-void mdstat_wait_fd(int fd, const sigset_t *sigmask)
+int mdstat_wait_fd(int fd, const sigset_t *sigmask)
 {
 	fd_set fds, rfds;
 	int maxfd = 0;
@@ -348,7 +348,11 @@ void mdstat_wait_fd(int fd, const sigset_t *sigmask)
 
 	if (fd >= 0) {
 		struct stat stb;
-		fstat(fd, &stb);
+
+		if (fstat(fd, &stb) != 0) {
+			pr_err("fstat failed: %s\n", strerror(errno));
+			return 1;
+		}
 		if ((stb.st_mode & S_IFMT) == S_IFREG)
 			/* Must be a /proc or /sys fd, so expect
 			 * POLLPRI
@@ -367,6 +371,8 @@ void mdstat_wait_fd(int fd, const sigset_t *sigmask)
 
 	pselect(maxfd + 1, &rfds, NULL, &fds,
 		NULL, sigmask);
+
+	return 0;
 }
 
 int mddev_busy(char *devnm)
diff --git a/super-ddf.c b/super-ddf.c
index dc8e512f..3c9eadf7 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -1060,7 +1060,11 @@ static int load_ddf_local(int fd, struct ddf_super *super,
 		     0);
 	dl->devname = devname ? xstrdup(devname) : NULL;
 
-	fstat(fd, &stb);
+	if (fstat(fd, &stb) != 0) {
+		pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
+		free(dl);
+		return 1;
+	}
 	dl->major = major(stb.st_rdev);
 	dl->minor = minor(stb.st_rdev);
 	dl->next = super->dlist;
@@ -2854,7 +2858,10 @@ static int add_to_super_ddf(struct supertype *st,
 	/* This is device numbered dk->number.  We need to create
 	 * a phys_disk entry and a more detailed disk_data entry.
 	 */
-	fstat(fd, &stb);
+	if (fstat(fd, &stb) != 0) {
+		pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
+		return 1;
+	}
 	n = find_unused_pde(ddf);
 	if (n == DDF_NOTFOUND) {
 		pr_err("No free slot in array, cannot add disk\n");
diff --git a/super-intel.c b/super-intel.c
index 83ddc000..1836be76 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -4164,7 +4164,11 @@ load_imsm_disk(int fd, struct intel_super *super, char *devname, int keep_fd)
 
 	dl = xcalloc(1, sizeof(*dl));
 
-	fstat(fd, &stb);
+	if (fstat(fd, &stb) != 0) {
+		pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
+		free(dl);
+		return 2;
+	}
 	dl->major = major(stb.st_rdev);
 	dl->minor = minor(stb.st_rdev);
 	dl->next = super->disks;
@@ -5910,7 +5914,10 @@ static int add_to_super_imsm(struct supertype *st, mdu_disk_info_t *dk,
 	if (super->current_vol >= 0)
 		return add_to_super_imsm_volume(st, dk, fd, devname);
 
-	fstat(fd, &stb);
+	if (fstat(fd, &stb) != 0) {
+		pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
+		return 1;
+	}
 	dd = xcalloc(sizeof(*dd), 1);
 	dd->major = major(stb.st_rdev);
 	dd->minor = minor(stb.st_rdev);
-- 
2.29.2

Re: [PATCH] Fix return value from fstat calls

[Tkaczyk, Mariusz]

+ linux raid

Mariusz

On 11.08.2021 15:03, Tkaczyk, Mariusz wrote:
> On 10.08.2021 17:15, Nigel Croxon wrote:
>> To meet requirements of Common Criteria certification vulnerablility
>> assessment. Static code analysis has been run and found the following
>> errors:
>> check_return: Calling "fstat(fd, &dstb)" without checking return value.
>> This library function may fail and return an error code.
>>
>> Changes are to add a test to the return value from fstat calls.
>>
> 
> Hi Nigel,
> You introduce three different errors, repeated many times across code.
> Could you make it generic using function or macro?
> 
>> diff --git a/Assemble.c b/Assemble.c
>> index 0df46244..cae3224b 100644
>> --- a/Assemble.c
>> +++ b/Assemble.c
>> @@ -649,7 +649,14 @@ static int load_devices(struct devs *devices, char *devmap,
>>               /* prepare useful information in info structures */
>>               struct stat stb2;
>>               int err;
>> -            fstat(mdfd, &stb2);
>> +
>> +            if (fstat(mdfd, &stb2) != 0) {
>> +                pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
>> +                close(mdfd);
>> +                free(devices);
>> +                free(devmap);
>> +                return -1;
>> +            }
> another new case with direct error handling, could you use goto statement?
> 
> 
>> @@ -760,7 +767,17 @@ static int load_devices(struct devs *devices, char *devmap,
>>               tst->ss->getinfo_super(tst, content, devmap + devcnt * 
>> content->array.raid_disks);
>>           }
>> -        fstat(dfd, &stb);
>> +        if (fstat(dfd, &stb) != 0) {
>> +            pr_err("fstat failed for %s: %s - aborting\n",devname, 
>> strerror(errno));
>> +            close(dfd);
>> +            close(mdfd);
>> +            free(devices);
>> +            free(devmap);
>> +            tst->ss->free_super(tst);
>> +            free(tst);
>> +            *stp = st;
>> +            return -1;
>> +        }
> Same here, I know that it is implemented this way, but we should take care about
> code quality, this is our common interest.
> 
>> diff --git a/Dump.c b/Dump.c
>> index 736bcb60..d1a8bb86 100644
>> --- a/Dump.c
>> +++ b/Dump.c
>> @@ -112,7 +112,14 @@ int Dump_metadata(char *dev, char *dir, struct context *c,
>>       }
>>       if (c->verbose >= 0)
>>           printf("%s saved as %s.\n", dev, fname);
>> -    fstat(fd, &dstb);
>> +
>> +    if (fstat(fd, &dstb) != 0) {
>> +        pr_err("fstat failed from %s: %s\n",fname, strerror(errno));
>> +        close(fd);
>> +        close(fl);
>> +        free(fname);
>> +        return 1;
>> +    }
> Same here.
> 
>> @@ -200,7 +207,11 @@ int Restore_metadata(char *dev, char *dir, struct context 
>> *c,
>>           char *chosen = NULL;
>>           unsigned int chosen_inode = 0;
>> -        fstat(fd, &dstb);
>> +        if (fstat(fd, &dstb) != 0) {
>> +            pr_err("fstat failed for %s: %s\n",dev, strerror(errno));
>> +            close(fd);
>> +            return 1;
>> +        }
>>           while (d && (de = readdir(d)) != NULL) {
>>               if (de->d_name[0] == '.')
>> diff --git a/Grow.c b/Grow.c
>> index 7506ab46..4c3ec9c1 100644
>> --- a/Grow.c
>> +++ b/Grow.c
>> @@ -1163,9 +1163,17 @@ int reshape_open_backup_file(char *backup_file,
>>        * way this will not notice, but it is better than
>>        * nothing.
>>        */
>> -    fstat(*fdlist, &stb);
>> +    if (fstat(*fdlist, &stb) != 0) {
>> +        pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
>> +        close(*fdlist);
>> +        return 0;
>> +    }
>>       dev = stb.st_dev;
>> -    fstat(fd, &stb);
>> +    if (fstat(fd, &stb) != 0) {
>> +        pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
>> +        close(*fdlist);
>> +        return 0;
>> +    }
>>       if (stb.st_rdev == dev) {
>>           pr_err("backup file must NOT be on the array being reshaped.\n");
>>           close(*fdlist);
> Same error handling duplicated.
> 
> Thanks,
> Mariusz
> 

[PATCH V2] Fix return value from fstat calls

[Nigel Croxon]

To meet requirements of Common Criteria certification vulnerablility
assessment. Static code analysis has been run and found the following
errors:
check_return: Calling "fstat(fd, &dstb)" without checking return value.
This library function may fail and return an error code.

Changes are to add a test to the return value from fstat calls.

V2 - Assemble.c,Dump.c,Grow.c combined error paths into one path.

Signed-off-by: Nigel Croxon <ncroxon@redhat.com>
---
 Assemble.c    | 49 ++++++++++++++++++++++---------------------------
 Dump.c        | 30 +++++++++++++++++-------------
 Grow.c        | 16 ++++++++++++----
 config.c      |  5 ++++-
 managemon.c   |  9 ++++++---
 mdadm.h       |  2 +-
 mdstat.c      | 10 ++++++++--
 super-ddf.c   | 11 +++++++++--
 super-intel.c | 11 +++++++++--
 9 files changed, 88 insertions(+), 55 deletions(-)

diff --git a/Assemble.c b/Assemble.c
index 0df4624..1bd16f0 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -649,7 +649,11 @@ static int load_devices(struct devs *devices, char *devmap,
 			/* prepare useful information in info structures */
 			struct stat stb2;
 			int err;
-			fstat(mdfd, &stb2);
+
+			if (fstat(mdfd, &stb2) != 0) {
+				pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
+				goto out;
+			}
 
 			if (strcmp(c->update, "uuid") == 0 && !ident->uuid_set)
 				random_uuid((__u8 *)ident->uuid);
@@ -657,10 +661,7 @@ static int load_devices(struct devs *devices, char *devmap,
 			if (strcmp(c->update, "ppl") == 0 &&
 			    ident->bitmap_fd >= 0) {
 				pr_err("PPL is not compatible with bitmap\n");
-				close(mdfd);
-				free(devices);
-				free(devmap);
-				return -1;
+				goto out;
 			}
 
 			dfd = dev_open(devname,
@@ -673,13 +674,9 @@ static int load_devices(struct devs *devices, char *devmap,
 				       devname);
 				if (dfd >= 0)
 					close(dfd);
-				close(mdfd);
-				free(devices);
-				free(devmap);
 				tst->ss->free_super(tst);
 				free(tst);
-				*stp = st;
-				return -1;
+				goto next;
 			}
 			tst->ss->getinfo_super(tst, content, devmap + devcnt * content->array.raid_disks);
 
@@ -712,13 +709,8 @@ static int load_devices(struct devs *devices, char *devmap,
 					pr_err("--update=%s not understood for %s metadata\n",
 					       c->update, tst->ss->name);
 				tst->ss->free_super(tst);
-				free(tst);
-				close(mdfd);
 				close(dfd);
-				free(devices);
-				free(devmap);
-				*stp = st;
-				return -1;
+				goto next;
 			}
 			if (strcmp(c->update, "uuid")==0 &&
 			    !ident->uuid_set) {
@@ -749,18 +741,17 @@ static int load_devices(struct devs *devices, char *devmap,
 				       devname);
 				if (dfd >= 0)
 					close(dfd);
-				close(mdfd);
-				free(devices);
-				free(devmap);
 				tst->ss->free_super(tst);
 				free(tst);
-				*stp = st;
-				return -1;
+				goto next;
 			}
 			tst->ss->getinfo_super(tst, content, devmap + devcnt * content->array.raid_disks);
 		}
 
-		fstat(dfd, &stb);
+		if (fstat(dfd, &stb) != 0) {
+			pr_err("fstat failed for %s: %s - aborting\n",devname, strerror(errno));
+			goto next;
+		}
 		close(dfd);
 
 		if (c->verbose > 0)
@@ -840,11 +831,7 @@ static int load_devices(struct devs *devices, char *devmap,
 				       inargv ? "the list" :
 				       "the\n      DEVICE list in mdadm.conf"
 					);
-				close(mdfd);
-				free(devices);
-				free(devmap);
-				*stp = st;
-				return -1;
+				goto next;
 			}
 			if (best[i] == -1 || (devices[best[i]].i.events
 					      < devices[devcnt].i.events))
@@ -860,6 +847,14 @@ static int load_devices(struct devs *devices, char *devmap,
 	*bestp = best;
 	*stp = st;
 	return devcnt;
+
+next:
+	*stp = st;
+out:
+	close(mdfd);
+	free(devices);
+	free(devmap);
+	return -1;
 }
 
 static int force_array(struct mdinfo *content,
diff --git a/Dump.c b/Dump.c
index 736bcb6..6881dc1 100644
--- a/Dump.c
+++ b/Dump.c
@@ -95,26 +95,21 @@ int Dump_metadata(char *dev, char *dir, struct context *c,
 	if (ftruncate(fl, size) < 0) {
 		pr_err("failed to set size of dump file: %s\n",
 		       strerror(errno));
-		close(fd);
-		close(fl);
-		free(fname);
-		return 1;
+		goto out;
 	}
 
 	if (st->ss->copy_metadata(st, fd, fl) != 0) {
 		pr_err("Failed to copy metadata from %s to %s\n",
 		       dev, fname);
-		close(fd);
-		close(fl);
 		unlink(fname);
-		free(fname);
-		return 1;
+		goto out;
 	}
 	if (c->verbose >= 0)
 		printf("%s saved as %s.\n", dev, fname);
-	fstat(fd, &dstb);
-	close(fd);
-	close(fl);
+	if (fstat(fd, &dstb) != 0) {
+		pr_err("fstat failed from %s: %s\n",fname, strerror(errno));
+		goto out;
+	}
 	if ((dstb.st_mode & S_IFMT) != S_IFBLK) {
 		/* Not a block device, so cannot create links */
 		free(fname);
@@ -153,6 +148,12 @@ int Dump_metadata(char *dev, char *dir, struct context *c,
 	closedir(dirp);
 	free(fname);
 	return 0;
+
+out:
+	close(fd);
+	close(fl);
+	free(fname);
+	return 1;
 }
 
 int Restore_metadata(char *dev, char *dir, struct context *c,
@@ -200,8 +201,11 @@ int Restore_metadata(char *dev, char *dir, struct context *c,
 		char *chosen = NULL;
 		unsigned int chosen_inode = 0;
 
-		fstat(fd, &dstb);
-
+		if (fstat(fd, &dstb) != 0) {
+			pr_err("fstat failed for %s: %s\n",dev, strerror(errno));
+			close(fd);
+			return 1;
+		}
 		while (d && (de = readdir(d)) != NULL) {
 			if (de->d_name[0] == '.')
 				continue;
diff --git a/Grow.c b/Grow.c
index 7506ab4..1f3b0c1 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1163,13 +1163,18 @@ int reshape_open_backup_file(char *backup_file,
 	 * way this will not notice, but it is better than
 	 * nothing.
 	 */
-	fstat(*fdlist, &stb);
+	if (fstat(*fdlist, &stb) != 0) {
+		pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
+		goto out;
+	}
 	dev = stb.st_dev;
-	fstat(fd, &stb);
+	if (fstat(fd, &stb) != 0) {
+		pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
+		goto out;
+	}
 	if (stb.st_rdev == dev) {
 		pr_err("backup file must NOT be on the array being reshaped.\n");
-		close(*fdlist);
-		return 0;
+		goto out;
 	}
 
 	memset(buf, 0, 512);
@@ -1195,6 +1200,9 @@ int reshape_open_backup_file(char *backup_file,
 	}
 
 	return 1;
+out:
+	close(*fdlist);
+	return 0;
 }
 
 unsigned long compute_backup_blocks(int nchunk, int ochunk,
diff --git a/config.c b/config.c
index 9c72545..ad0e02f 100644
--- a/config.c
+++ b/config.c
@@ -803,7 +803,10 @@ void conf_file_or_dir(FILE *f)
 	struct dirent *dp;
 	struct fname *list = NULL;
 
-	fstat(fileno(f), &st);
+	if (fstat(fileno(f), &st) != 0) {
+		pr_err("fstat failed: %s\n", strerror(errno));
+		return;
+	}
 	if (S_ISREG(st.st_mode))
 		conf_file(f);
 	else if (!S_ISDIR(st.st_mode))
diff --git a/managemon.c b/managemon.c
index 200cf83..d943f2c 100644
--- a/managemon.c
+++ b/managemon.c
@@ -894,6 +894,7 @@ void do_manager(struct supertype *container)
 {
 	struct mdstat_ent *mdstat;
 	sigset_t set;
+	int rtn;
 
 	sigprocmask(SIG_UNBLOCK, NULL, &set);
 	sigdelset(&set, SIGUSR1);
@@ -926,9 +927,11 @@ void do_manager(struct supertype *container)
 		if (sigterm)
 			wakeup_monitor();
 
-		if (update_queue == NULL)
-			mdstat_wait_fd(container->sock, &set);
-		else
+		if (update_queue == NULL) {
+			rtn = mdstat_wait_fd(container->sock, &set);
+			if (rtn)
+				exit(0);
+		} else
 			/* If an update is happening, just wait for signal */
 			pselect(0, NULL, NULL, NULL, NULL, &set);
 	} while(1);
diff --git a/mdadm.h b/mdadm.h
index 8f8841d..1273029 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -637,7 +637,7 @@ extern struct mdstat_ent *mdstat_read(int hold, int start);
 extern void mdstat_close(void);
 extern void free_mdstat(struct mdstat_ent *ms);
 extern int mdstat_wait(int seconds);
-extern void mdstat_wait_fd(int fd, const sigset_t *sigmask);
+extern int mdstat_wait_fd(int fd, const sigset_t *sigmask);
 extern int mddev_busy(char *devnm);
 extern struct mdstat_ent *mdstat_by_component(char *name);
 extern struct mdstat_ent *mdstat_by_subdev(char *subdev, char *container);
diff --git a/mdstat.c b/mdstat.c
index 2fd792c..8664c0e 100644
--- a/mdstat.c
+++ b/mdstat.c
@@ -336,7 +336,7 @@ int mdstat_wait(int seconds)
 	return select(maxfd + 1, NULL, NULL, &fds, &tm);
 }
 
-void mdstat_wait_fd(int fd, const sigset_t *sigmask)
+int mdstat_wait_fd(int fd, const sigset_t *sigmask)
 {
 	fd_set fds, rfds;
 	int maxfd = 0;
@@ -348,7 +348,11 @@ void mdstat_wait_fd(int fd, const sigset_t *sigmask)
 
 	if (fd >= 0) {
 		struct stat stb;
-		fstat(fd, &stb);
+
+		if (fstat(fd, &stb) != 0) {
+			pr_err("fstat failed: %s\n", strerror(errno));
+			return 1;
+		}
 		if ((stb.st_mode & S_IFMT) == S_IFREG)
 			/* Must be a /proc or /sys fd, so expect
 			 * POLLPRI
@@ -367,6 +371,8 @@ void mdstat_wait_fd(int fd, const sigset_t *sigmask)
 
 	pselect(maxfd + 1, &rfds, NULL, &fds,
 		NULL, sigmask);
+
+	return 0;
 }
 
 int mddev_busy(char *devnm)
diff --git a/super-ddf.c b/super-ddf.c
index dc8e512..3c9eadf 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -1060,7 +1060,11 @@ static int load_ddf_local(int fd, struct ddf_super *super,
 		     0);
 	dl->devname = devname ? xstrdup(devname) : NULL;
 
-	fstat(fd, &stb);
+	if (fstat(fd, &stb) != 0) {
+		pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
+		free(dl);
+		return 1;
+	}
 	dl->major = major(stb.st_rdev);
 	dl->minor = minor(stb.st_rdev);
 	dl->next = super->dlist;
@@ -2854,7 +2858,10 @@ static int add_to_super_ddf(struct supertype *st,
 	/* This is device numbered dk->number.  We need to create
 	 * a phys_disk entry and a more detailed disk_data entry.
 	 */
-	fstat(fd, &stb);
+	if (fstat(fd, &stb) != 0) {
+		pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
+		return 1;
+	}
 	n = find_unused_pde(ddf);
 	if (n == DDF_NOTFOUND) {
 		pr_err("No free slot in array, cannot add disk\n");
diff --git a/super-intel.c b/super-intel.c
index 83ddc00..1836be7 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -4164,7 +4164,11 @@ load_imsm_disk(int fd, struct intel_super *super, char *devname, int keep_fd)
 
 	dl = xcalloc(1, sizeof(*dl));
 
-	fstat(fd, &stb);
+	if (fstat(fd, &stb) != 0) {
+		pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
+		free(dl);
+		return 2;
+	}
 	dl->major = major(stb.st_rdev);
 	dl->minor = minor(stb.st_rdev);
 	dl->next = super->disks;
@@ -5910,7 +5914,10 @@ static int add_to_super_imsm(struct supertype *st, mdu_disk_info_t *dk,
 	if (super->current_vol >= 0)
 		return add_to_super_imsm_volume(st, dk, fd, devname);
 
-	fstat(fd, &stb);
+	if (fstat(fd, &stb) != 0) {
+		pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
+		return 1;
+	}
 	dd = xcalloc(sizeof(*dd), 1);
 	dd->major = major(stb.st_rdev);
 	dd->minor = minor(stb.st_rdev);
-- 
2.29.2

Re: [PATCH V2] Fix return value from fstat calls

[NeilBrown]

On Thu, 12 Aug 2021, Nigel Croxon wrote:
> To meet requirements of Common Criteria certification vulnerablility
> assessment. Static code analysis has been run and found the following
> errors:
> check_return: Calling "fstat(fd, &dstb)" without checking return value.
> This library function may fail and return an error code.

In what circumstances might it fail and return an error code?

NeilBrown

Re: [PATCH V2] Fix return value from fstat calls

[Tkaczyk, Mariusz]

Hi Nigel,
On 11.08.2021 21:09, Nigel Croxon wrote:

> +				pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
> +		pr_err("fstat failed from %s: %s\n",fname, strerror(errno));
Typo, I guess that you want to use "for".

> +			pr_err("fstat failed for %s: %s\n",dev, strerror(errno));
> +		pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
> +		pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
> +		pr_err("fstat failed: %s\n", strerror(errno));
> +			pr_err("fstat failed: %s\n", strerror(errno));
> +		pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
> +		pr_err("fstat failed for %s: %s\n",devname, strerror(errno));

> +		pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
> +		pr_err("fstat failed for %s: %s\n",devname, strerror(errno));

You are using similar error message across code. If you think that printing
error in this case is worth to be added then please define wrapper for fstat
with error message and use that.
Current solution is typo friendly (and I found one) and breaks DNRY rule.
Any modern C IDE is able to find references- result is same as in search via
string in case of error.

Mariusz

Re: [PATCH V2] Fix return value from fstat calls

[NeilBrown]

On Thu, 12 Aug 2021, Nigel Croxon wrote:
> On Wednesday, Aug 11, 2021 at 6:53 PM, NeilBrown <neilb@suse.de (mailto:neilb@suse.de)> wrote:
> On Thu, 12 Aug 2021, Nigel Croxon wrote:
> > To meet requirements of Common Criteria certification vulnerablility
> > assessment. Static code analysis has been run and found the following
> > errors:
> > check_return: Calling "fstat(fd, &dstb)" without checking return value.
> > This library function may fail and return an error code.
> 
> In what circumstances might it fail and return an error code?
> 
> NeilBrown
> 
> Hello Neil,
> 
> The fstat() function will fail if:
> [EBADF] - The fildes argument is not a valid file descriptor.

But we never pass an invalid file descriptor

And you didn't list "EFAULT", but of course we never pass an invalid
memory address either.

> [EIO] - An I/O error occurred while reading from the file system.

fstat() doesn't do IO, it just reports data from the cache.

> [EOVERFLOW] - The file size in bytes or the number of blocks allocated to the file or the file serial number cannot be represented correctly in the structure pointed to by buf.
> 
> The fstat() function may fail if:
> 
> [EOVERFLOW] - One of the values is too large to store into the structure pointed to by the buf argument.
> 

Those don't happen in practice for the fstat() calls that mdadm makes
either.

I think this patch is adding noise to the source code without actually
providing any real value.  I would much prefer that if you really feel
there is value, then just add a wrapper:

int  safe_fstat(....)
{
    int ret = fstat(.....);
    char message[]="mdadm: fstat failed, so aborting\n"
    if (ret == 0)
         return 0;
    write(2, message, sizeof(message)-1);
    exit(1);
}

Then just change every "fstat" in the code that bothers you to
"safe_fstat()".

This approach of adding pointless checks because some static analysis
tool thinks you should is not an approach that I approve of.

But, of course, it is up to Jes what patches he accepts...

NeilBrown

Re: [PATCH V2] Fix return value from fstat calls

[Tkaczyk, Mariusz]

On 13.08.2021 01:23, NeilBrown wrote:
>>
>> The fstat() function will fail if:
>> [EBADF] - The fildes argument is not a valid file descriptor.
> 
> But we never pass an invalid file descriptor
> 
We can't guarantee that. There is always a minimal chance to pass
wrong/invalid argument caused by bug somewhere else in mdadm logic.
I think that handling such case is reasonable from security point
of view but agree that it could be a dead check (if everything is
well implemented).

> And you didn't list "EFAULT", but of course we never pass an invalid
> memory address either.

As before, we can't guarantee it, too. For now it seems to be well handled
but implementation may change and vulnerability might be missed during
review. Is safer to handle that some way.

>> [EIO] - An I/O error occurred while reading from the file system.
> 
> fstat() doesn't do IO, it just reports data from the cache.
> 
>> [EOVERFLOW] - The file size in bytes or the number of blocks allocated to the file or the file serial number cannot be represented correctly in the structure pointed to by buf.
>>
>> The fstat() function may fail if:
>>
>> [EOVERFLOW] - One of the values is too large to store into the structure pointed to by the buf argument.
>>
> 
> Those don't happen in practice for the fstat() calls that mdadm makes
> either.
> 
Agree, but it could be changed.

> I think this patch is adding noise to the source code without actually
> providing any real value.  I would much prefer that if you really feel
> there is value, then just add a wrapper:
> 
> int  safe_fstat(....)
> {
>      int ret = fstat(.....);
>      char message[]="mdadm: fstat failed, so aborting\n"
>      if (ret == 0)
>           return 0;
>      write(2, message, sizeof(message)-1);
>      exit(1);
> }
> 
> Then just change every "fstat" in the code that bothers you to
> "safe_fstat()".
> 
> This approach of adding pointless checks because some static analysis
> tool thinks you should is not an approach that I approve of.

Maybe It won't be useful for users but it may help developers to avoid
trivial mistakes. As you told, if everything is fine then check is dead.
In my opinion any error handling is better than nothing.

Anyway, I think that there is a lot of more dangerous lines.

Regards,
Mariusz

Re: [PATCH V2] Fix return value from fstat calls

[NeilBrown]

On Fri, 13 Aug 2021, Tkaczyk, Mariusz wrote:
> 
> Maybe It won't be useful for users but it may help developers to avoid
> trivial mistakes. As you told, if everything is fine then check is dead.
> In my opinion any error handling is better than nothing.

Error handling that is buggy, or that is hard to maintain is not better
than nothing.  If I can't guarantee that we never pass a bad file
descriptor, then you cannot guarantee that the error handling has no
bugs.   Less code generally means less bugs.

Any attempt to try to handle an error that should not be able to happen
other than crashing is fairly pointless - you cannot guess the real
cause, so you cannot know how to repair.  Just printing a message and
continuing could be as bad as not checking the error.

NeilBrown

Re: [PATCH V2] Fix return value from fstat calls

[Tkaczyk, Mariusz]

On 13.08.2021 09:19, NeilBrown wrote:
> On Fri, 13 Aug 2021, Tkaczyk, Mariusz wrote:
>>

> Error handling that is buggy, or that is hard to maintain is not better
> than nothing.  If I can't guarantee that we never pass a bad file
> descriptor, then you cannot guarantee that the error handling has no
> bugs.   Less code generally means less bugs.
> 
> Any attempt to try to handle an error that should not be able to happen
> other than crashing is fairly pointless - you cannot guess the real
> cause, so you cannot know how to repair.  Just printing a message and
> continuing could be as bad as not checking the error.
> As error handling, I meant any error verification. It doesn't indicate
that we should return status and end gracefully. exit() is elegant
solution in this case, totally agree.

Thanks,
Mariusz

Re: [PATCH V2] Fix return value from fstat calls

[Jes Sorensen]

On 8/13/21 3:45 AM, Tkaczyk, Mariusz wrote:
> On 13.08.2021 09:19, NeilBrown wrote:
>> On Fri, 13 Aug 2021, Tkaczyk, Mariusz wrote:
>>>
> 
>> Error handling that is buggy, or that is hard to maintain is not better
>> than nothing.  If I can't guarantee that we never pass a bad file
>> descriptor, then you cannot guarantee that the error handling has no
>> bugs.   Less code generally means less bugs.
>>
>> Any attempt to try to handle an error that should not be able to happen
>> other than crashing is fairly pointless - you cannot guess the real
>> cause, so you cannot know how to repair.  Just printing a message and
>> continuing could be as bad as not checking the error.
>> As error handling, I meant any error verification. It doesn't indicate
> that we should return status and end gracefully. exit() is elegant
> solution in this case, totally agree.

Just catching up here on this.

I totally agree that we need to work on catching errors and exiting
properly. It will also help returning error codes from this more silly
error handling cases to keep the certification people happy. This is a
much bigger job than just these checks though.

I don't think Nigel's patch is really harmful, but I don't think it adds
any real value either, without returning the actual error codes from
fstat and parsing them op the stack properly.

Jes