From 9368b3877c06061a602cb6638ee64f173e9b90d3 Mon Sep 17 00:00:00 2001 From: Richard Yao Date: Sun, 4 Dec 2022 17:42:43 -0500 Subject: [PATCH] Fix TOCTOU race in zpool_do_labelclear() Coverity reported a TOCTOU race in `zpool_do_labelclear()`. This is not believed to be a real security issue, but fixing it reduces the number of syscalls we do and will prevent other static analyzers from complaining about this. The code is expected to be equivalent. However, under rare circumstances, such as ELOOP, ENAMETOOLONG, ENOMEM, ENOTDIR and EOVERFLOW, we will display the error message that we currently display for the `open()` syscall rather than the one that we currently display for the `stat()` syscall. This is considered to be an improvement. Reported-by: Coverity (CID-1524188) Reviewed-by: Brian Behlendorf Signed-off-by: Richard Yao Closes #14575 --- cmd/zpool/zpool_main.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/cmd/zpool/zpool_main.c b/cmd/zpool/zpool_main.c index efb2d10e59..d2614411df 100644 --- a/cmd/zpool/zpool_main.c +++ b/cmd/zpool/zpool_main.c @@ -1288,7 +1288,6 @@ zpool_do_labelclear(int argc, char **argv) { char vdev[MAXPATHLEN]; char *name = NULL; - struct stat st; int c, fd = -1, ret = 0; nvlist_t *config; pool_state_t state; @@ -1321,14 +1320,20 @@ zpool_do_labelclear(int argc, char **argv) usage(B_FALSE); } + (void) strlcpy(vdev, argv[0], sizeof (vdev)); + /* - * Check if we were given absolute path and use it as is. + * If we cannot open an absolute path, we quit. * Otherwise if the provided vdev name doesn't point to a file, * try prepending expected disk paths and partition numbers. */ - (void) strlcpy(vdev, argv[0], sizeof (vdev)); - if (vdev[0] != '/' && stat(vdev, &st) != 0) { + if ((fd = open(vdev, O_RDWR)) < 0) { int error; + if (vdev[0] == '/') { + (void) fprintf(stderr, gettext("failed to open " + "%s: %s\n"), vdev, strerror(errno)); + return (1); + } error = zfs_resolve_shortname(argv[0], vdev, MAXPATHLEN); if (error == 0 && zfs_dev_is_whole_disk(vdev)) { @@ -1336,20 +1341,21 @@ zpool_do_labelclear(int argc, char **argv) error = ENOENT; } - if (error || (stat(vdev, &st) != 0)) { - (void) fprintf(stderr, gettext( - "failed to find device %s, try specifying absolute " - "path instead\n"), argv[0]); + if (error || ((fd = open(vdev, O_RDWR)) < 0)) { + if (errno == ENOENT) { + (void) fprintf(stderr, gettext( + "failed to find device %s, try " + "specifying absolute path instead\n"), + argv[0]); + return (1); + } + + (void) fprintf(stderr, gettext("failed to open %s:" + " %s\n"), vdev, strerror(errno)); return (1); } } - if ((fd = open(vdev, O_RDWR)) < 0) { - (void) fprintf(stderr, gettext("failed to open %s: %s\n"), - vdev, strerror(errno)); - return (1); - } - /* * Flush all dirty pages for the block device. This should not be * fatal when the device does not support BLKFLSBUF as would be the