[dm-devel] [PATCH v2 00/21] multipath-tools: coverity fixes

[dm-devel] [PATCH v2 00/21] multipath-tools: coverity fixes

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Hi Christophe, hi Ben,

I have started a new attempt to fix defects reported by coverity.
With this set (on top of the previously posted one), and a number
of false positive classifications, I have been able to bring the
defect count all the way down to zero.

Most of these patches fix defects reported by coverity. Some
fix other things that occured to me while working on this.

Changes v1 -> v2 (Ben Marzinski)

 - all: rebased to current "queue" branch
 - 05/21: If a broken designator is encountered in VPD 83, don't
     error out but try to go on as long as the designator is within
     limits of the VPD page
 - 06/21: adapt tests to the changes in 05/21, add some more
     (note: dropped Ben's "Reviewed-by:" on this one)
 - 08/21: reworked to take Ben's remarks into account

I didn't change 07/21 (see previous discussion).

Regards,
Martin

Martin Wilck (21):
  multipath tools: github workflows: add coverity workflow
  multipathd (coverity): check atexit() return value
  multipathd (coverity): terminate uxlsnr when polls allocation fails
  libmultipath: strbuf: add __get_strbuf_buf()
  libmultipath (coverity): improve input checking in parse_vpd_pg83
  multipath-tools: add tests for broken VPD page 83
  libmultipath: use strbuf in parse_vpd_pg83()
  libmultipath (coverity): fix tainted values in alua_rtpg.c
  multipath, multipathd: exit if bindings file is broken
  libmultipath (coverity): silence unchecked return value warning
  multipath: remove pointless code from getopt processing
  libmultipath (coverity): set umask before mkstemp
  multipathd (coverity): simplify set_oom_adj()
  kpartx: open /dev/loop-control only once
  kpartx: use opened loop device immediately
  kpartx: find_unused_loop_device(): add newlines
  multipathd (coverity): daemonize(): use dup2
  libmultipath (coverity): avoid sleeping in dm_mapname()
  libmultipath (coverity): Revert "setup_map: wait for pending path
    checkers to finish"
  libmultipath (coverity): check return values in dm_get_multipath()
  libmultipath: update_pathvec_from_dm: don't force DI_WWID

 .github/workflows/coverity.yaml       |  51 ++++++
 kpartx/kpartx.c                       |   4 +-
 kpartx/lopart.c                       | 100 +++++------
 kpartx/lopart.h                       |   3 +-
 libmultipath/alias.c                  |   4 +
 libmultipath/configure.c              |  63 +------
 libmultipath/devmapper.c              |  23 +--
 libmultipath/discovery.c              | 245 ++++++++++++++------------
 libmultipath/prioritizers/alua_rtpg.c |  13 +-
 libmultipath/prioritizers/alua_spc3.h |  43 ++++-
 libmultipath/propsel.c                |   2 +-
 libmultipath/strbuf.c                 |   5 +
 libmultipath/strbuf.h                 |  14 ++
 libmultipath/structs_vec.c            |   7 +-
 multipath/main.c                      |  13 +-
 multipathd/main.c                     |  91 +++++-----
 multipathd/uxlsnr.c                   |   1 +
 tests/vpd.c                           | 144 ++++++++++++++-
 18 files changed, 500 insertions(+), 326 deletions(-)
 create mode 100644 .github/workflows/coverity.yaml

-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 01/21] multipath tools: github workflows: add coverity workflow

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Add a workflow that triggers a coverity static analysis scan.
For now, this will only be done on a special branch called "coverity".
Pushing to that branch will trigger the workflow.

For this to work, 3 secrets need to be set in the Github repository:

COVERITY_SCAN_EMAIL: the email address for coverity/synopsis account
COVERITY_SCAN_TOKEN: the coverity / synopsis access token
COVERITY_SCAN_PROJECT: the coverity project, e.g. mwilck/multipath-tools

The workflow succeeds if upload of the coverity results was successful.
The analysis result will be emailed to the given address.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 .github/workflows/coverity.yaml | 51 +++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 .github/workflows/coverity.yaml

diff --git a/.github/workflows/coverity.yaml b/.github/workflows/coverity.yaml
new file mode 100644
index 0000000..a8b56d4
--- /dev/null
+++ b/.github/workflows/coverity.yaml
@@ -0,0 +1,51 @@
+name: coverity
+on:
+  push:
+    branches:
+      - coverity
+
+jobs:
+  upload-coverity-scan:
+    runs-on: ubuntu-20.04
+    steps:
+      - name: checkout
+        uses: actions/checkout@v2
+      - name: dependencies
+        run: >
+          sudo apt-get install --yes
+          gcc make pkg-config
+          libdevmapper-dev libreadline-dev libaio-dev libsystemd-dev
+          libudev-dev libjson-c-dev liburcu-dev libcmocka-dev
+      - name: download coverity
+        run: >
+          curl -o cov-analysis-linux64.tar.gz
+          --form token="$COV_TOKEN"
+          --form project="$COV_PROJECT"
+          https://scan.coverity.com/download/cxx/linux64
+        env:
+          COV_TOKEN: ${{ secrets.COVERITY_SCAN_TOKEN }}
+          COV_PROJECT:  ${{ secrets.COVERITY_SCAN_PROJECT }}
+      - name: unpack coverity
+        run: |
+          mkdir -p coverity
+          tar xfz cov-analysis-linux64.tar.gz --strip 1 -C coverity
+      - name: build with cov-build
+        run: >
+          PATH="$PWD/coverity/bin:$PATH"
+          cov-build --dir cov-int make -O -j"$(grep -c ^processor /proc/cpuinfo)"
+      - name: pack results
+        run: tar cfz multipath-tools.tgz cov-int
+      - name: submit results
+        run: >
+          curl
+          --form token="$COV_TOKEN"
+          --form email="$COV_EMAIL"
+          --form file="@multipath-tools.tgz"
+          --form version="${{ github.ref_name }}"
+          --form description="$(git describe --tags --match "0.*")"
+          --form project="$COV_PROJECT"
+          https://scan.coverity.com/builds
+        env:
+          COV_TOKEN: ${{ secrets.COVERITY_SCAN_TOKEN }}
+          COV_PROJECT:  ${{ secrets.COVERITY_SCAN_PROJECT }}
+          COV_EMAIL: ${{ secrets.COVERITY_SCAN_EMAIL }}
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 02/21] multipathd (coverity): check atexit() return value

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/multipath/main.c b/multipath/main.c
index 151cb93..cd234ac 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -837,7 +837,8 @@ main (int argc, char *argv[])
 	conf = get_multipath_config();
 	conf->retrigger_tries = 0;
 	conf->force_sync = 1;
-	atexit(cleanup_vecs);
+	if (atexit(cleanup_vecs))
+		condlog(1, "failed to register cleanup handler for vecs: %m");
 	while ((arg = getopt(argc, argv, ":adDcChl::eFfM:v:p:b:BrR:itTquUwW")) != EOF ) {
 		switch(arg) {
 		case 1: printf("optarg : %s\n",optarg);
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 03/21] multipathd (coverity): terminate uxlsnr when polls allocation fails

[mwilck]

From: Martin Wilck <mwilck@suse.com>

The polls array is dereferenced later on, so even if we are soon
to be cancelled, we should return immediately here.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/uxlsnr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index cd025ea..912ac3c 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -609,6 +609,7 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 	if (!polls) {
 		condlog(0, "uxsock: failed to allocate poll fds");
 		exit_daemon();
+		return NULL;
 	}
 	notify_fd = inotify_init1(IN_NONBLOCK);
 	if (notify_fd == -1) /* it's fine if notifications fail */
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 04/21] libmultipath: strbuf: add __get_strbuf_buf()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Add a function for internal use that allows direct manipulation
of the strbuf's content.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/strbuf.c |  5 +++++
 libmultipath/strbuf.h | 14 ++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/libmultipath/strbuf.c b/libmultipath/strbuf.c
index a24a57d..f654594 100644
--- a/libmultipath/strbuf.c
+++ b/libmultipath/strbuf.c
@@ -15,6 +15,11 @@
 
 static const char empty_str[] = "";
 
+char *__get_strbuf_buf(struct strbuf *buf)
+{
+	return buf->buf;
+}
+
 const char *get_strbuf_str(const struct strbuf *buf)
 {
 	return buf->buf ? buf->buf : empty_str;
diff --git a/libmultipath/strbuf.h b/libmultipath/strbuf.h
index 5903572..41d7d54 100644
--- a/libmultipath/strbuf.h
+++ b/libmultipath/strbuf.h
@@ -54,6 +54,20 @@ void free_strbuf(struct strbuf *buf);
  */
 struct strbuf *new_strbuf(void);
 
+/**
+ * get_strbuf_buf(): retrieve a pointer to the strbuf's buffer
+ * @param buf: a struct strbuf
+ * @returns: pointer to the string written to the strbuf so far.
+ *
+ * INTERNAL ONLY.
+ * DANGEROUS: Unlike the return value of get_strbuf_str(),
+ * this string can be written to, modifying the strbuf's content.
+ * USE WITH CAUTION.
+ * If @strbuf was never written to, the function returns NULL.
+ * The return value of this function must not be free()d.
+ */
+char *__get_strbuf_buf(struct strbuf *buf);
+
 /**
  * get_strbuf_str(): retrieve string from strbuf
  * @param buf: a struct strbuf
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 05/21] libmultipath (coverity): improve input checking in parse_vpd_pg83

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Check offsets and other obvious errors in the VPD83 data.

The original reason to do this was to fix "tained scalar"
warnings from coverity. But this doesn't suffice for coverity
without using a constant boundary (WWID_SIZE) for "len" in
parse_vpd_pg80() and for "vpd_len" in parse_vpd_pg83(), even
though the computed boundaries are tighter than the constant
ones.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 134 +++++++++++++++++++++++++--------------
 1 file changed, 88 insertions(+), 46 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 36ea7b3..977aed9 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -36,6 +36,8 @@
 #include "print.h"
 #include "strbuf.h"
 
+#define VPD_BUFLEN 4096
+
 struct vpd_vendor_page vpd_vendor_pages[VPD_VP_ARRAY_SIZE] = {
 	[VPD_VP_UNDEF]	= { 0x00, "undef" },
 	[VPD_VP_HP3PAR]	= { 0xc0, "hp3par" },
@@ -1086,6 +1088,8 @@ parse_vpd_pg80(const unsigned char *in, char *out, size_t out_len)
 	if (out_len == 0)
 		return 0;
 
+	if (len > WWID_SIZE)
+		len = WWID_SIZE;
 	/*
 	 * Strip leading and trailing whitespace
 	 */
@@ -1115,84 +1119,123 @@ parse_vpd_pg83(const unsigned char *in, size_t in_len,
 	const unsigned char *d;
 	const unsigned char *vpd = NULL;
 	size_t len, vpd_len, i;
-	int vpd_type, prio = -1, naa_prio;
+	int vpd_type, prio = -1;
+	int err = -ENODATA;
+
+	/* Need space at least for one digit */
+	if (out_len <= 1)
+		return 0;
 
 	d = in + 4;
-	while (d < in + in_len) {
+	while (d <= in + in_len - 4) {
+		bool invalid = false;
+		int new_prio = -1;
+
 		/* Select 'association: LUN' */
-		if ((d[1] & 0x30) != 0) {
-			d += d[3] + 4;
-			continue;
-		}
+		if ((d[1] & 0x30) == 0x30) {
+			invalid = true;
+			goto next_designator;
+		} else if ((d[1] & 0x30) != 0x00)
+			goto next_designator;
+
 		switch (d[1] & 0xf) {
+			unsigned char good_len;
 		case 0x3:
 			/* NAA: Prio 5 */
 			switch (d[4] >> 4) {
 			case 6:
 				/* IEEE Registered Extended: Prio 8 */
-				naa_prio = 8;
+				new_prio = 8;
+				good_len = 16;
 				break;
 			case 5:
 				/* IEEE Registered: Prio 7 */
-				naa_prio = 7;
+				new_prio = 7;
+				good_len = 8;
 				break;
 			case 2:
 				/* IEEE Extended: Prio 6 */
-				naa_prio = 6;
+				new_prio = 6;
+				good_len = 8;
 				break;
 			case 3:
 				/* IEEE Locally assigned: Prio 1 */
-				naa_prio = 1;
+				new_prio = 1;
+				good_len = 8;
 				break;
 			default:
 				/* Default: no priority */
-				naa_prio = -1;
+				good_len = 0xff;
 				break;
 			}
-			if (prio < naa_prio) {
-				prio = naa_prio;
-				vpd = d;
-			}
+
+			invalid = good_len == 0xff || good_len != d[3];
 			break;
 		case 0x2:
 			/* EUI-64: Prio 4 */
-			if (prio < 4) {
-				prio = 4;
-				vpd = d;
-			}
+			invalid = (d[3] != 8 && d[3] != 12 && d[3] != 16);
+			new_prio = 4;
 			break;
 		case 0x8:
 			/* SCSI Name: Prio 3 */
-			if (memcmp(d + 4, "eui.", 4) &&
-			    memcmp(d + 4, "naa.", 4) &&
-			    memcmp(d + 4, "iqn.", 4))
-				break;
-			if (prio < 3) {
-				prio = 3;
-				vpd = d;
-			}
+			invalid = (d[3] < 4 || (memcmp(d + 4, "eui.", 4) &&
+						memcmp(d + 4, "naa.", 4) &&
+						memcmp(d + 4, "iqn.", 4)));
+			new_prio = 3;
 			break;
 		case 0x1:
 			/* T-10 Vendor ID: Prio 2 */
-			if (prio < 2) {
-				prio = 2;
-				vpd = d;
-			}
+			invalid = (d[3] < 8);
+			new_prio = 2;
 			break;
+		case 0xa:
+			condlog(2, "%s: UUID identifiers not yet supported",
+				__func__);
+			break;
+		default:
+			invalid = true;
+			break;
+		}
+
+	next_designator:
+		if (d + d[3] + 4 - in > (ssize_t)in_len) {
+			condlog(2, "%s: device descriptor length overflow: %zd > %zu",
+				__func__, d + d[3] + 4 - in, in_len);
+			err = -EOVERFLOW;
+			break;
+		} else if (invalid) {
+			condlog(2, "%s: invalid device designator at offset %zd: %02x%02x%02x%02x",
+				__func__, d - in, d[0], d[1], d[2], d[3]);
+			/*
+			 * We checked above that the next offset is within limits.
+			 * Proceed, fingers crossed.
+			 */
+			err = -EINVAL;
+		} else if (new_prio > prio) {
+			vpd = d;
+			prio = new_prio;
 		}
 		d += d[3] + 4;
 	}
 
 	if (prio <= 0)
-		return -ENODATA;
-	/* Need space at least for one digit */
-	else if (out_len <= 1)
-		return 0;
+		return err;
+
+	if (d != in + in_len)
+		/* Should this be fatal? (overflow covered above) */
+		condlog(2, "%s: warning: last descriptor end %zd != VPD length %zu",
+			__func__, d - in, in_len);
 
 	len = 0;
 	vpd_type = vpd[1] & 0xf;
 	vpd_len = vpd[3];
 	vpd += 4;
+	/* untaint vpd_len for coverity */
+	if (vpd_len > WWID_SIZE) {
+		condlog(1, "%s: suspicious designator length %zu truncated to %u",
+			__func__, vpd_len, WWID_SIZE);
+		vpd_len = WWID_SIZE;
+	}
 	if (vpd_type == 0x2 || vpd_type == 0x3) {
 		size_t i;
 
@@ -1206,10 +1249,6 @@ parse_vpd_pg83(const unsigned char *in, size_t in_len,
 		for (i = 0; i < vpd_len; i++)
 			len += sprintf(out + len,
 				       "%02x", vpd[i]);
-	} else if (vpd_type == 0x8 && vpd_len < 4) {
-		condlog(1, "%s: VPD length %zu too small for designator type 8",
-			__func__, vpd_len);
-		return -EINVAL;
 	} else if (vpd_type == 0x8) {
 		if (!memcmp("eui.", vpd, 4))
 			out[0] =  '2';
@@ -1316,11 +1355,12 @@ parse_vpd_c0_hp3par(const unsigned char *in, size_t in_len,
 static int
 get_vpd_sysfs (struct udev_device *parent, int pg, char * str, int maxlen)
 {
-	int len, buff_len;
-	unsigned char buff[4096];
+	int len;
+	size_t buff_len;
+	unsigned char buff[VPD_BUFLEN];
 
-	memset(buff, 0x0, 4096);
-	if (!parent || sysfs_get_vpd(parent, pg, buff, 4096) <= 0) {
+	memset(buff, 0x0, VPD_BUFLEN);
+	if (!parent || sysfs_get_vpd(parent, pg, buff, VPD_BUFLEN) <= 0) {
 		condlog(3, "failed to read sysfs vpd pg%02x", pg);
 		return -EINVAL;
 	}
@@ -1331,8 +1371,10 @@ get_vpd_sysfs (struct udev_device *parent, int pg, char * str, int maxlen)
 		return -ENODATA;
 	}
 	buff_len = get_unaligned_be16(&buff[2]) + 4;
-	if (buff_len > 4096)
+	if (buff_len > VPD_BUFLEN) {
 		condlog(3, "vpd pg%02x page truncated", pg);
+		buff_len = VPD_BUFLEN;
+	}
 
 	if (pg == 0x80)
 		len = parse_vpd_pg80(buff, str, maxlen);
@@ -1376,7 +1418,7 @@ bool
 is_vpd_page_supported(int fd, int pg)
 {
 	int i, len;
-	unsigned char buff[4096];
+	unsigned char buff[VPD_BUFLEN];
 
 	len = fetch_vpd_page(fd, 0x00, buff, sizeof(buff));
 	if (len < 0)
@@ -1392,7 +1434,7 @@ int
 get_vpd_sgio (int fd, int pg, int vend_id, char * str, int maxlen)
 {
 	int len, buff_len;
-	unsigned char buff[4096];
+	unsigned char buff[VPD_BUFLEN];
 
 	buff_len = fetch_vpd_page(fd, pg, buff, sizeof(buff));
 	if (buff_len < 0)
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 05/21] libmultipath (coverity): improve input checking in parse_vpd_pg83

[Benjamin Marzinski]

On Wed, Dec 01, 2021 at 01:36:34PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Check offsets and other obvious errors in the VPD83 data.
> 
> The original reason to do this was to fix "tained scalar"
> warnings from coverity. But this doesn't suffice for coverity
> without using a constant boundary (WWID_SIZE) for "len" in
> parse_vpd_pg80() and for "vpd_len" in parse_vpd_pg83(), even
> though the computed boundaries are tighter than the constant
> ones.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/discovery.c | 134 +++++++++++++++++++++++++--------------
>  1 file changed, 88 insertions(+), 46 deletions(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 36ea7b3..977aed9 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -36,6 +36,8 @@
>  #include "print.h"
>  #include "strbuf.h"
>  
> +#define VPD_BUFLEN 4096
> +
>  struct vpd_vendor_page vpd_vendor_pages[VPD_VP_ARRAY_SIZE] = {
>  	[VPD_VP_UNDEF]	= { 0x00, "undef" },
>  	[VPD_VP_HP3PAR]	= { 0xc0, "hp3par" },
> @@ -1086,6 +1088,8 @@ parse_vpd_pg80(const unsigned char *in, char *out, size_t out_len)
>  	if (out_len == 0)
>  		return 0;
>  
> +	if (len > WWID_SIZE)
> +		len = WWID_SIZE;
>  	/*
>  	 * Strip leading and trailing whitespace
>  	 */
> @@ -1115,84 +1119,123 @@ parse_vpd_pg83(const unsigned char *in, size_t in_len,
>  	const unsigned char *d;
>  	const unsigned char *vpd = NULL;
>  	size_t len, vpd_len, i;
> -	int vpd_type, prio = -1, naa_prio;
> +	int vpd_type, prio = -1;
> +	int err = -ENODATA;
> +
> +	/* Need space at least for one digit */
> +	if (out_len <= 1)
> +		return 0;
>  
>  	d = in + 4;
> -	while (d < in + in_len) {
> +	while (d <= in + in_len - 4) {
> +		bool invalid = false;
> +		int new_prio = -1;
> +
>  		/* Select 'association: LUN' */
> -		if ((d[1] & 0x30) != 0) {
> -			d += d[3] + 4;
> -			continue;
> -		}
> +		if ((d[1] & 0x30) == 0x30) {
> +			invalid = true;
> +			goto next_designator;
> +		} else if ((d[1] & 0x30) != 0x00)
> +			goto next_designator;
> +
>  		switch (d[1] & 0xf) {
> +			unsigned char good_len;
>  		case 0x3:
>  			/* NAA: Prio 5 */
>  			switch (d[4] >> 4) {
>  			case 6:
>  				/* IEEE Registered Extended: Prio 8 */
> -				naa_prio = 8;
> +				new_prio = 8;
> +				good_len = 16;
>  				break;
>  			case 5:
>  				/* IEEE Registered: Prio 7 */
> -				naa_prio = 7;
> +				new_prio = 7;
> +				good_len = 8;
>  				break;
>  			case 2:
>  				/* IEEE Extended: Prio 6 */
> -				naa_prio = 6;
> +				new_prio = 6;
> +				good_len = 8;
>  				break;
>  			case 3:
>  				/* IEEE Locally assigned: Prio 1 */
> -				naa_prio = 1;
> +				new_prio = 1;
> +				good_len = 8;
>  				break;
>  			default:
>  				/* Default: no priority */
> -				naa_prio = -1;
> +				good_len = 0xff;
>  				break;
>  			}
> -			if (prio < naa_prio) {
> -				prio = naa_prio;
> -				vpd = d;
> -			}
> +
> +			invalid = good_len == 0xff || good_len != d[3];
>  			break;
>  		case 0x2:
>  			/* EUI-64: Prio 4 */
> -			if (prio < 4) {
> -				prio = 4;
> -				vpd = d;
> -			}
> +			invalid = (d[3] != 8 && d[3] != 12 && d[3] != 16);
> +			new_prio = 4;
>  			break;
>  		case 0x8:
>  			/* SCSI Name: Prio 3 */
> -			if (memcmp(d + 4, "eui.", 4) &&
> -			    memcmp(d + 4, "naa.", 4) &&
> -			    memcmp(d + 4, "iqn.", 4))
> -				break;
> -			if (prio < 3) {
> -				prio = 3;
> -				vpd = d;
> -			}
> +			invalid = (d[3] < 4 || (memcmp(d + 4, "eui.", 4) &&
> +						memcmp(d + 4, "naa.", 4) &&
> +						memcmp(d + 4, "iqn.", 4)));
> +			new_prio = 3;
>  			break;
>  		case 0x1:
>  			/* T-10 Vendor ID: Prio 2 */
> -			if (prio < 2) {
> -				prio = 2;
> -				vpd = d;
> -			}
> +			invalid = (d[3] < 8);
> +			new_prio = 2;
>  			break;
> +		case 0xa:
> +			condlog(2, "%s: UUID identifiers not yet supported",
> +				__func__);
> +			break;
> +		default:
> +			invalid = true;
> +			break;
> +		}
> +
> +	next_designator:
> +		if (d + d[3] + 4 - in > (ssize_t)in_len) {
> +			condlog(2, "%s: device descriptor length overflow: %zd > %zu",
> +				__func__, d + d[3] + 4 - in, in_len);
> +			err = -EOVERFLOW;
> +			break;
> +		} else if (invalid) {
> +			condlog(2, "%s: invalid device designator at offset %zd: %02x%02x%02x%02x",
> +				__func__, d - in, d[0], d[1], d[2], d[3]);
> +			/*
> +			 * We checked above that the next offset is within limits.
> +			 * Proceed, fingers crossed.
> +			 */
> +			err = -EINVAL;
> +		} else if (new_prio > prio) {
> +			vpd = d;
> +			prio = new_prio;
>  		}
>  		d += d[3] + 4;
>  	}
>  
>  	if (prio <= 0)
> -		return -ENODATA;
> -	/* Need space at least for one digit */
> -	else if (out_len <= 1)
> -		return 0;
> +		return err;
> +
> +	if (d != in + in_len)
> +		/* Should this be fatal? (overflow covered above) */
> +		condlog(2, "%s: warning: last descriptor end %zd != VPD length %zu",
> +			__func__, d - in, in_len);
>  
>  	len = 0;
>  	vpd_type = vpd[1] & 0xf;
>  	vpd_len = vpd[3];
>  	vpd += 4;
> +	/* untaint vpd_len for coverity */
> +	if (vpd_len > WWID_SIZE) {
> +		condlog(1, "%s: suspicious designator length %zu truncated to %u",
> +			__func__, vpd_len, WWID_SIZE);
> +		vpd_len = WWID_SIZE;
> +	}
>  	if (vpd_type == 0x2 || vpd_type == 0x3) {
>  		size_t i;
>  
> @@ -1206,10 +1249,6 @@ parse_vpd_pg83(const unsigned char *in, size_t in_len,
>  		for (i = 0; i < vpd_len; i++)
>  			len += sprintf(out + len,
>  				       "%02x", vpd[i]);
> -	} else if (vpd_type == 0x8 && vpd_len < 4) {
> -		condlog(1, "%s: VPD length %zu too small for designator type 8",
> -			__func__, vpd_len);
> -		return -EINVAL;
>  	} else if (vpd_type == 0x8) {
>  		if (!memcmp("eui.", vpd, 4))
>  			out[0] =  '2';
> @@ -1316,11 +1355,12 @@ parse_vpd_c0_hp3par(const unsigned char *in, size_t in_len,
>  static int
>  get_vpd_sysfs (struct udev_device *parent, int pg, char * str, int maxlen)
>  {
> -	int len, buff_len;
> -	unsigned char buff[4096];
> +	int len;
> +	size_t buff_len;
> +	unsigned char buff[VPD_BUFLEN];
>  
> -	memset(buff, 0x0, 4096);
> -	if (!parent || sysfs_get_vpd(parent, pg, buff, 4096) <= 0) {
> +	memset(buff, 0x0, VPD_BUFLEN);
> +	if (!parent || sysfs_get_vpd(parent, pg, buff, VPD_BUFLEN) <= 0) {
>  		condlog(3, "failed to read sysfs vpd pg%02x", pg);
>  		return -EINVAL;
>  	}
> @@ -1331,8 +1371,10 @@ get_vpd_sysfs (struct udev_device *parent, int pg, char * str, int maxlen)
>  		return -ENODATA;
>  	}
>  	buff_len = get_unaligned_be16(&buff[2]) + 4;
> -	if (buff_len > 4096)
> +	if (buff_len > VPD_BUFLEN) {
>  		condlog(3, "vpd pg%02x page truncated", pg);
> +		buff_len = VPD_BUFLEN;
> +	}
>  
>  	if (pg == 0x80)
>  		len = parse_vpd_pg80(buff, str, maxlen);
> @@ -1376,7 +1418,7 @@ bool
>  is_vpd_page_supported(int fd, int pg)
>  {
>  	int i, len;
> -	unsigned char buff[4096];
> +	unsigned char buff[VPD_BUFLEN];
>  
>  	len = fetch_vpd_page(fd, 0x00, buff, sizeof(buff));
>  	if (len < 0)
> @@ -1392,7 +1434,7 @@ int
>  get_vpd_sgio (int fd, int pg, int vend_id, char * str, int maxlen)
>  {
>  	int len, buff_len;
> -	unsigned char buff[4096];
> +	unsigned char buff[VPD_BUFLEN];
>  
>  	buff_len = fetch_vpd_page(fd, pg, buff, sizeof(buff));
>  	if (buff_len < 0)
> -- 
> 2.33.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 06/21] multipath-tools: add tests for broken VPD page 83

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Add some tests for the consistency checks in the VPD.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 tests/vpd.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 143 insertions(+), 1 deletion(-)

diff --git a/tests/vpd.c b/tests/vpd.c
index 8e730d3..a7d2092 100644
--- a/tests/vpd.c
+++ b/tests/vpd.c
@@ -9,6 +9,7 @@
 #include <stdint.h>
 #include <stdlib.h>
 #include <unistd.h>
+#include <errno.h>
 #include <stdarg.h>
 #include <stddef.h>
 #include <setjmp.h>
@@ -291,7 +292,9 @@ static int create_vpd83(unsigned char *buf, size_t bufsiz, const char *id,
 	unsigned char *desc;
 	int n = 0;
 
-	memset(buf, 0, bufsiz);
+	/* Fill with large number, which will cause length overflow */
+	memset(buf, 0xed, bufsiz);
+	buf[0] = 0;
 	buf[1] = 0x83;
 
 	desc = buf + 4;
@@ -500,6 +503,27 @@ static void test_vpd_naa_ ## naa ## _ ## wlen(void **state)             \
 			    test_id, vt->wwid);				\
 }
 
+/**
+ * test_cpd_naa_NAA_badlen_BAD() - test detection of bad length fields
+ * @NAA:	Network Name Authority (2, 3, 5, 16)
+ * @BAD:        Value for designator length field
+ * @ERR:        Expected error code
+ */
+#define make_test_vpd_naa_badlen(NAA, BAD, ERR)			\
+static void test_vpd_naa_##NAA##_badlen_##BAD(void **state)	\
+{								\
+	struct vpdtest *vt = *state;					\
+	int n, ret;							\
+									\
+	n = create_vpd83(vt->vpdbuf, sizeof(vt->vpdbuf), test_id, 3, NAA, 0); \
+									\
+	vt->vpdbuf[7] = BAD;						\
+	will_return(__wrap_ioctl, n);					\
+	will_return(__wrap_ioctl, vt->vpdbuf);				\
+	ret = get_vpd_sgio(10, 0x83, 0, vt->wwid, 40);			\
+	assert_int_equal(-ret, -ERR);					\
+}
+
 /**
  * test_vpd_eui_LEN_WLEN() - test code for VPD 83, EUI64
  * @LEN:	EUI64 length (8, 12, or 16)
@@ -532,6 +556,31 @@ static void test_vpd_eui_ ## len ## _ ## wlen ## _ ## sml(void **state)	\
 			    test_id, vt->wwid);				\
 }
 
+/**
+ * test_cpd_eui_LEN_badlen_BAD() - test detection of bad length fields
+ * @NAA:	correct length(8, 12, 16)
+ * @BAD:        value for designator length field
+ * @ERR:        expected error code
+ */
+#define make_test_vpd_eui_badlen(LEN, BAD, ERR)			\
+static void test_vpd_eui_badlen_##LEN##_##BAD(void **state)	\
+{								\
+	struct vpdtest *vt = *state;					\
+	int n, ret;							\
+									\
+	n = create_vpd83(vt->vpdbuf, sizeof(vt->vpdbuf), test_id, 2, 0, LEN); \
+									\
+	vt->vpdbuf[7] = BAD;						\
+	will_return(__wrap_ioctl, n);					\
+	will_return(__wrap_ioctl, vt->vpdbuf);				\
+	ret = get_vpd_sgio(10, 0x83, 0, vt->wwid, 40);			\
+	assert_int_equal(ret, ERR);					\
+	if (ERR >= 0)							\
+		assert_correct_wwid("test_vpd_eui_badlen_"#LEN"_"#BAD,	\
+			    2 * BAD + 1, ret, '2', 0, true,		\
+			    test_id, vt->wwid);				\
+}
+
 /**
  * test_vpd80_SIZE_LEN_WLEN() - test code for VPD 80
  * @SIZE, @LEN:	see create_vpd80()
@@ -621,6 +670,17 @@ make_test_vpd_eui(8, 17, 0);
 make_test_vpd_eui(8, 16, 0);
 make_test_vpd_eui(8, 10, 0);
 
+make_test_vpd_eui_badlen(8, 8, 17);
+/* Invalid entry, length overflow */
+make_test_vpd_eui_badlen(8, 12, -EOVERFLOW);
+make_test_vpd_eui_badlen(8, 9, -EOVERFLOW);
+/* invalid entry, no length overflow, but no full next entry */
+make_test_vpd_eui_badlen(8, 7, -EINVAL);
+make_test_vpd_eui_badlen(8, 5, -EINVAL);
+/* invalid entry, length of next one readable but too long */
+make_test_vpd_eui_badlen(8, 4, -EOVERFLOW);
+make_test_vpd_eui_badlen(8, 0, -EOVERFLOW);
+
 /* 96 bit, WWID size: 26 */
 make_test_vpd_eui(12, 32, 0);
 make_test_vpd_eui(12, 26, 0);
@@ -628,12 +688,38 @@ make_test_vpd_eui(12, 25, 0);
 make_test_vpd_eui(12, 20, 0);
 make_test_vpd_eui(12, 10, 0);
 
+make_test_vpd_eui_badlen(12, 12, 25);
+make_test_vpd_eui_badlen(12, 16, -EOVERFLOW);
+make_test_vpd_eui_badlen(12, 13, -EOVERFLOW);
+/* invalid entry, no length overflow, but no full next entry */
+make_test_vpd_eui_badlen(12, 11, -EINVAL);
+make_test_vpd_eui_badlen(12, 9, -EINVAL);
+/* non-fatal - valid 8-byte descriptor */
+make_test_vpd_eui_badlen(12, 8, 17);
+/* invalid entry, length of next one readable but too long */
+make_test_vpd_eui_badlen(12, 7, -EOVERFLOW);
+make_test_vpd_eui_badlen(12, 0, -EOVERFLOW);
+
 /* 128 bit, WWID size: 34 */
 make_test_vpd_eui(16, 40, 0);
 make_test_vpd_eui(16, 34, 0);
 make_test_vpd_eui(16, 33, 0);
 make_test_vpd_eui(16, 20, 0);
 
+make_test_vpd_eui_badlen(16, 16, 33);
+make_test_vpd_eui_badlen(16, 17, -EOVERFLOW);
+make_test_vpd_eui_badlen(16, 15, -EINVAL);
+make_test_vpd_eui_badlen(16, 13, -EINVAL);
+/* non-fatal - valid 12-byte descriptor */
+make_test_vpd_eui_badlen(16, 12, 25);
+/* invalid entry, length of next one readable but too long */
+make_test_vpd_eui_badlen(16, 11, -EOVERFLOW);
+/* non-fatal - valid 8-byte descriptor */
+make_test_vpd_eui_badlen(16, 8, 17);
+/* invalid entry, length of next one readable but too long */
+make_test_vpd_eui_badlen(16, 7, -EOVERFLOW);
+make_test_vpd_eui_badlen(16, 0, -EOVERFLOW);
+
 /* NAA IEEE registered extended (36), WWID size: 34 */
 make_test_vpd_naa(6, 40);
 make_test_vpd_naa(6, 34);
@@ -641,12 +727,33 @@ make_test_vpd_naa(6, 33);
 make_test_vpd_naa(6, 32);
 make_test_vpd_naa(6, 20);
 
+/* NAA IEEE registered extended with bad designator length */
+make_test_vpd_naa_badlen(6, 16, 33);
+/* offset overflow */
+make_test_vpd_naa_badlen(6, 17, -EOVERFLOW);
+/* invalid entry, no length overflow, but no full next entry */
+make_test_vpd_naa_badlen(6, 15, -EINVAL);
+/* invalid entry, length of next one readable but too long */
+make_test_vpd_naa_badlen(6, 8, -EOVERFLOW);
+make_test_vpd_naa_badlen(6, 0, -EOVERFLOW);
+
 /* NAA IEEE registered (35), WWID size: 18 */
 make_test_vpd_naa(5, 20);
 make_test_vpd_naa(5, 18);
 make_test_vpd_naa(5, 17);
 make_test_vpd_naa(5, 16);
 
+/* NAA IEEE registered with bad designator length */
+make_test_vpd_naa_badlen(5, 8, 17);
+/* offset overflow */
+make_test_vpd_naa_badlen(5, 16, -EOVERFLOW);
+make_test_vpd_naa_badlen(5, 9, -EOVERFLOW);
+/* invalid entry, no length overflow, but no full next entry */
+make_test_vpd_naa_badlen(5, 7, -EINVAL);
+/* invalid entry, length of next one readable but too long */
+make_test_vpd_naa_badlen(5, 4, -EOVERFLOW);
+make_test_vpd_naa_badlen(5, 0, -EOVERFLOW);
+
 /* NAA local (33), WWID size: 18 */
 make_test_vpd_naa(3, 20);
 make_test_vpd_naa(3, 18);
@@ -741,24 +848,59 @@ static int test_vpd(void)
 		cmocka_unit_test(test_vpd_eui_8_17_0),
 		cmocka_unit_test(test_vpd_eui_8_16_0),
 		cmocka_unit_test(test_vpd_eui_8_10_0),
+		cmocka_unit_test(test_vpd_eui_badlen_8_8),
+		cmocka_unit_test(test_vpd_eui_badlen_8_12),
+		cmocka_unit_test(test_vpd_eui_badlen_8_9),
+		cmocka_unit_test(test_vpd_eui_badlen_8_7),
+		cmocka_unit_test(test_vpd_eui_badlen_8_5),
+		cmocka_unit_test(test_vpd_eui_badlen_8_4),
+		cmocka_unit_test(test_vpd_eui_badlen_8_0),
 		cmocka_unit_test(test_vpd_eui_12_32_0),
 		cmocka_unit_test(test_vpd_eui_12_26_0),
 		cmocka_unit_test(test_vpd_eui_12_25_0),
 		cmocka_unit_test(test_vpd_eui_12_20_0),
 		cmocka_unit_test(test_vpd_eui_12_10_0),
+		cmocka_unit_test(test_vpd_eui_badlen_12_12),
+		cmocka_unit_test(test_vpd_eui_badlen_12_16),
+		cmocka_unit_test(test_vpd_eui_badlen_12_13),
+		cmocka_unit_test(test_vpd_eui_badlen_12_11),
+		cmocka_unit_test(test_vpd_eui_badlen_12_9),
+		cmocka_unit_test(test_vpd_eui_badlen_12_8),
+		cmocka_unit_test(test_vpd_eui_badlen_12_7),
+		cmocka_unit_test(test_vpd_eui_badlen_12_0),
 		cmocka_unit_test(test_vpd_eui_16_40_0),
 		cmocka_unit_test(test_vpd_eui_16_34_0),
 		cmocka_unit_test(test_vpd_eui_16_33_0),
 		cmocka_unit_test(test_vpd_eui_16_20_0),
+		cmocka_unit_test(test_vpd_eui_badlen_16_16),
+		cmocka_unit_test(test_vpd_eui_badlen_16_17),
+		cmocka_unit_test(test_vpd_eui_badlen_16_15),
+		cmocka_unit_test(test_vpd_eui_badlen_16_13),
+		cmocka_unit_test(test_vpd_eui_badlen_16_12),
+		cmocka_unit_test(test_vpd_eui_badlen_16_11),
+		cmocka_unit_test(test_vpd_eui_badlen_16_8),
+		cmocka_unit_test(test_vpd_eui_badlen_16_7),
+		cmocka_unit_test(test_vpd_eui_badlen_16_0),
 		cmocka_unit_test(test_vpd_naa_6_40),
 		cmocka_unit_test(test_vpd_naa_6_34),
 		cmocka_unit_test(test_vpd_naa_6_33),
 		cmocka_unit_test(test_vpd_naa_6_32),
 		cmocka_unit_test(test_vpd_naa_6_20),
+		cmocka_unit_test(test_vpd_naa_6_badlen_16),
+		cmocka_unit_test(test_vpd_naa_6_badlen_15),
+		cmocka_unit_test(test_vpd_naa_6_badlen_8),
+		cmocka_unit_test(test_vpd_naa_6_badlen_17),
+		cmocka_unit_test(test_vpd_naa_6_badlen_0),
 		cmocka_unit_test(test_vpd_naa_5_20),
 		cmocka_unit_test(test_vpd_naa_5_18),
 		cmocka_unit_test(test_vpd_naa_5_17),
 		cmocka_unit_test(test_vpd_naa_5_16),
+		cmocka_unit_test(test_vpd_naa_5_badlen_8),
+		cmocka_unit_test(test_vpd_naa_5_badlen_7),
+		cmocka_unit_test(test_vpd_naa_5_badlen_4),
+		cmocka_unit_test(test_vpd_naa_5_badlen_16),
+		cmocka_unit_test(test_vpd_naa_5_badlen_9),
+		cmocka_unit_test(test_vpd_naa_5_badlen_0),
 		cmocka_unit_test(test_vpd_naa_3_20),
 		cmocka_unit_test(test_vpd_naa_3_18),
 		cmocka_unit_test(test_vpd_naa_3_17),
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 06/21] multipath-tools: add tests for broken VPD page 83

[Benjamin Marzinski]

On Wed, Dec 01, 2021 at 01:36:35PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Add some tests for the consistency checks in the VPD.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  tests/vpd.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 143 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/vpd.c b/tests/vpd.c
> index 8e730d3..a7d2092 100644
> --- a/tests/vpd.c
> +++ b/tests/vpd.c
> @@ -9,6 +9,7 @@
>  #include <stdint.h>
>  #include <stdlib.h>
>  #include <unistd.h>
> +#include <errno.h>
>  #include <stdarg.h>
>  #include <stddef.h>
>  #include <setjmp.h>
> @@ -291,7 +292,9 @@ static int create_vpd83(unsigned char *buf, size_t bufsiz, const char *id,
>  	unsigned char *desc;
>  	int n = 0;
>  
> -	memset(buf, 0, bufsiz);
> +	/* Fill with large number, which will cause length overflow */
> +	memset(buf, 0xed, bufsiz);
> +	buf[0] = 0;
>  	buf[1] = 0x83;
>  
>  	desc = buf + 4;
> @@ -500,6 +503,27 @@ static void test_vpd_naa_ ## naa ## _ ## wlen(void **state)             \
>  			    test_id, vt->wwid);				\
>  }
>  
> +/**
> + * test_cpd_naa_NAA_badlen_BAD() - test detection of bad length fields
> + * @NAA:	Network Name Authority (2, 3, 5, 16)
> + * @BAD:        Value for designator length field
> + * @ERR:        Expected error code
> + */
> +#define make_test_vpd_naa_badlen(NAA, BAD, ERR)			\
> +static void test_vpd_naa_##NAA##_badlen_##BAD(void **state)	\
> +{								\
> +	struct vpdtest *vt = *state;					\
> +	int n, ret;							\
> +									\
> +	n = create_vpd83(vt->vpdbuf, sizeof(vt->vpdbuf), test_id, 3, NAA, 0); \
> +									\
> +	vt->vpdbuf[7] = BAD;						\
> +	will_return(__wrap_ioctl, n);					\
> +	will_return(__wrap_ioctl, vt->vpdbuf);				\
> +	ret = get_vpd_sgio(10, 0x83, 0, vt->wwid, 40);			\
> +	assert_int_equal(-ret, -ERR);					\
> +}
> +
>  /**
>   * test_vpd_eui_LEN_WLEN() - test code for VPD 83, EUI64
>   * @LEN:	EUI64 length (8, 12, or 16)
> @@ -532,6 +556,31 @@ static void test_vpd_eui_ ## len ## _ ## wlen ## _ ## sml(void **state)	\
>  			    test_id, vt->wwid);				\
>  }
>  
> +/**
> + * test_cpd_eui_LEN_badlen_BAD() - test detection of bad length fields
> + * @NAA:	correct length(8, 12, 16)
> + * @BAD:        value for designator length field
> + * @ERR:        expected error code
> + */
> +#define make_test_vpd_eui_badlen(LEN, BAD, ERR)			\
> +static void test_vpd_eui_badlen_##LEN##_##BAD(void **state)	\
> +{								\
> +	struct vpdtest *vt = *state;					\
> +	int n, ret;							\
> +									\
> +	n = create_vpd83(vt->vpdbuf, sizeof(vt->vpdbuf), test_id, 2, 0, LEN); \
> +									\
> +	vt->vpdbuf[7] = BAD;						\
> +	will_return(__wrap_ioctl, n);					\
> +	will_return(__wrap_ioctl, vt->vpdbuf);				\
> +	ret = get_vpd_sgio(10, 0x83, 0, vt->wwid, 40);			\
> +	assert_int_equal(ret, ERR);					\
> +	if (ERR >= 0)							\
> +		assert_correct_wwid("test_vpd_eui_badlen_"#LEN"_"#BAD,	\
> +			    2 * BAD + 1, ret, '2', 0, true,		\
> +			    test_id, vt->wwid);				\
> +}
> +
>  /**
>   * test_vpd80_SIZE_LEN_WLEN() - test code for VPD 80
>   * @SIZE, @LEN:	see create_vpd80()
> @@ -621,6 +670,17 @@ make_test_vpd_eui(8, 17, 0);
>  make_test_vpd_eui(8, 16, 0);
>  make_test_vpd_eui(8, 10, 0);
>  
> +make_test_vpd_eui_badlen(8, 8, 17);
> +/* Invalid entry, length overflow */
> +make_test_vpd_eui_badlen(8, 12, -EOVERFLOW);
> +make_test_vpd_eui_badlen(8, 9, -EOVERFLOW);
> +/* invalid entry, no length overflow, but no full next entry */
> +make_test_vpd_eui_badlen(8, 7, -EINVAL);
> +make_test_vpd_eui_badlen(8, 5, -EINVAL);
> +/* invalid entry, length of next one readable but too long */
> +make_test_vpd_eui_badlen(8, 4, -EOVERFLOW);
> +make_test_vpd_eui_badlen(8, 0, -EOVERFLOW);
> +
>  /* 96 bit, WWID size: 26 */
>  make_test_vpd_eui(12, 32, 0);
>  make_test_vpd_eui(12, 26, 0);
> @@ -628,12 +688,38 @@ make_test_vpd_eui(12, 25, 0);
>  make_test_vpd_eui(12, 20, 0);
>  make_test_vpd_eui(12, 10, 0);
>  
> +make_test_vpd_eui_badlen(12, 12, 25);
> +make_test_vpd_eui_badlen(12, 16, -EOVERFLOW);
> +make_test_vpd_eui_badlen(12, 13, -EOVERFLOW);
> +/* invalid entry, no length overflow, but no full next entry */
> +make_test_vpd_eui_badlen(12, 11, -EINVAL);
> +make_test_vpd_eui_badlen(12, 9, -EINVAL);
> +/* non-fatal - valid 8-byte descriptor */
> +make_test_vpd_eui_badlen(12, 8, 17);
> +/* invalid entry, length of next one readable but too long */
> +make_test_vpd_eui_badlen(12, 7, -EOVERFLOW);
> +make_test_vpd_eui_badlen(12, 0, -EOVERFLOW);
> +
>  /* 128 bit, WWID size: 34 */
>  make_test_vpd_eui(16, 40, 0);
>  make_test_vpd_eui(16, 34, 0);
>  make_test_vpd_eui(16, 33, 0);
>  make_test_vpd_eui(16, 20, 0);
>  
> +make_test_vpd_eui_badlen(16, 16, 33);
> +make_test_vpd_eui_badlen(16, 17, -EOVERFLOW);
> +make_test_vpd_eui_badlen(16, 15, -EINVAL);
> +make_test_vpd_eui_badlen(16, 13, -EINVAL);
> +/* non-fatal - valid 12-byte descriptor */
> +make_test_vpd_eui_badlen(16, 12, 25);
> +/* invalid entry, length of next one readable but too long */
> +make_test_vpd_eui_badlen(16, 11, -EOVERFLOW);
> +/* non-fatal - valid 8-byte descriptor */
> +make_test_vpd_eui_badlen(16, 8, 17);
> +/* invalid entry, length of next one readable but too long */
> +make_test_vpd_eui_badlen(16, 7, -EOVERFLOW);
> +make_test_vpd_eui_badlen(16, 0, -EOVERFLOW);
> +
>  /* NAA IEEE registered extended (36), WWID size: 34 */
>  make_test_vpd_naa(6, 40);
>  make_test_vpd_naa(6, 34);
> @@ -641,12 +727,33 @@ make_test_vpd_naa(6, 33);
>  make_test_vpd_naa(6, 32);
>  make_test_vpd_naa(6, 20);
>  
> +/* NAA IEEE registered extended with bad designator length */
> +make_test_vpd_naa_badlen(6, 16, 33);
> +/* offset overflow */
> +make_test_vpd_naa_badlen(6, 17, -EOVERFLOW);
> +/* invalid entry, no length overflow, but no full next entry */
> +make_test_vpd_naa_badlen(6, 15, -EINVAL);
> +/* invalid entry, length of next one readable but too long */
> +make_test_vpd_naa_badlen(6, 8, -EOVERFLOW);
> +make_test_vpd_naa_badlen(6, 0, -EOVERFLOW);
> +
>  /* NAA IEEE registered (35), WWID size: 18 */
>  make_test_vpd_naa(5, 20);
>  make_test_vpd_naa(5, 18);
>  make_test_vpd_naa(5, 17);
>  make_test_vpd_naa(5, 16);
>  
> +/* NAA IEEE registered with bad designator length */
> +make_test_vpd_naa_badlen(5, 8, 17);
> +/* offset overflow */
> +make_test_vpd_naa_badlen(5, 16, -EOVERFLOW);
> +make_test_vpd_naa_badlen(5, 9, -EOVERFLOW);
> +/* invalid entry, no length overflow, but no full next entry */
> +make_test_vpd_naa_badlen(5, 7, -EINVAL);
> +/* invalid entry, length of next one readable but too long */
> +make_test_vpd_naa_badlen(5, 4, -EOVERFLOW);
> +make_test_vpd_naa_badlen(5, 0, -EOVERFLOW);
> +
>  /* NAA local (33), WWID size: 18 */
>  make_test_vpd_naa(3, 20);
>  make_test_vpd_naa(3, 18);
> @@ -741,24 +848,59 @@ static int test_vpd(void)
>  		cmocka_unit_test(test_vpd_eui_8_17_0),
>  		cmocka_unit_test(test_vpd_eui_8_16_0),
>  		cmocka_unit_test(test_vpd_eui_8_10_0),
> +		cmocka_unit_test(test_vpd_eui_badlen_8_8),
> +		cmocka_unit_test(test_vpd_eui_badlen_8_12),
> +		cmocka_unit_test(test_vpd_eui_badlen_8_9),
> +		cmocka_unit_test(test_vpd_eui_badlen_8_7),
> +		cmocka_unit_test(test_vpd_eui_badlen_8_5),
> +		cmocka_unit_test(test_vpd_eui_badlen_8_4),
> +		cmocka_unit_test(test_vpd_eui_badlen_8_0),
>  		cmocka_unit_test(test_vpd_eui_12_32_0),
>  		cmocka_unit_test(test_vpd_eui_12_26_0),
>  		cmocka_unit_test(test_vpd_eui_12_25_0),
>  		cmocka_unit_test(test_vpd_eui_12_20_0),
>  		cmocka_unit_test(test_vpd_eui_12_10_0),
> +		cmocka_unit_test(test_vpd_eui_badlen_12_12),
> +		cmocka_unit_test(test_vpd_eui_badlen_12_16),
> +		cmocka_unit_test(test_vpd_eui_badlen_12_13),
> +		cmocka_unit_test(test_vpd_eui_badlen_12_11),
> +		cmocka_unit_test(test_vpd_eui_badlen_12_9),
> +		cmocka_unit_test(test_vpd_eui_badlen_12_8),
> +		cmocka_unit_test(test_vpd_eui_badlen_12_7),
> +		cmocka_unit_test(test_vpd_eui_badlen_12_0),
>  		cmocka_unit_test(test_vpd_eui_16_40_0),
>  		cmocka_unit_test(test_vpd_eui_16_34_0),
>  		cmocka_unit_test(test_vpd_eui_16_33_0),
>  		cmocka_unit_test(test_vpd_eui_16_20_0),
> +		cmocka_unit_test(test_vpd_eui_badlen_16_16),
> +		cmocka_unit_test(test_vpd_eui_badlen_16_17),
> +		cmocka_unit_test(test_vpd_eui_badlen_16_15),
> +		cmocka_unit_test(test_vpd_eui_badlen_16_13),
> +		cmocka_unit_test(test_vpd_eui_badlen_16_12),
> +		cmocka_unit_test(test_vpd_eui_badlen_16_11),
> +		cmocka_unit_test(test_vpd_eui_badlen_16_8),
> +		cmocka_unit_test(test_vpd_eui_badlen_16_7),
> +		cmocka_unit_test(test_vpd_eui_badlen_16_0),
>  		cmocka_unit_test(test_vpd_naa_6_40),
>  		cmocka_unit_test(test_vpd_naa_6_34),
>  		cmocka_unit_test(test_vpd_naa_6_33),
>  		cmocka_unit_test(test_vpd_naa_6_32),
>  		cmocka_unit_test(test_vpd_naa_6_20),
> +		cmocka_unit_test(test_vpd_naa_6_badlen_16),
> +		cmocka_unit_test(test_vpd_naa_6_badlen_15),
> +		cmocka_unit_test(test_vpd_naa_6_badlen_8),
> +		cmocka_unit_test(test_vpd_naa_6_badlen_17),
> +		cmocka_unit_test(test_vpd_naa_6_badlen_0),
>  		cmocka_unit_test(test_vpd_naa_5_20),
>  		cmocka_unit_test(test_vpd_naa_5_18),
>  		cmocka_unit_test(test_vpd_naa_5_17),
>  		cmocka_unit_test(test_vpd_naa_5_16),
> +		cmocka_unit_test(test_vpd_naa_5_badlen_8),
> +		cmocka_unit_test(test_vpd_naa_5_badlen_7),
> +		cmocka_unit_test(test_vpd_naa_5_badlen_4),
> +		cmocka_unit_test(test_vpd_naa_5_badlen_16),
> +		cmocka_unit_test(test_vpd_naa_5_badlen_9),
> +		cmocka_unit_test(test_vpd_naa_5_badlen_0),
>  		cmocka_unit_test(test_vpd_naa_3_20),
>  		cmocka_unit_test(test_vpd_naa_3_18),
>  		cmocka_unit_test(test_vpd_naa_3_17),
> -- 
> 2.33.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 07/21] libmultipath: use strbuf in parse_vpd_pg83()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

By using the strbuf API, the code gets more readable, as we need
less output size checks.

While at it, avoid a possible crash by passing a NULL pointer
to memchr().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 111 +++++++++++++++++----------------------
 1 file changed, 48 insertions(+), 63 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 977aed9..7d939ae 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1121,6 +1121,7 @@ parse_vpd_pg83(const unsigned char *in, size_t in_len,
 	size_t len, vpd_len, i;
 	int vpd_type, prio = -1;
 	int err = -ENODATA;
+	STRBUF_ON_STACK(buf);
 
 	/* Need space at least for one digit */
 	if (out_len <= 1)
@@ -1239,92 +1240,76 @@ parse_vpd_pg83(const unsigned char *in, size_t in_len,
 	if (vpd_type == 0x2 || vpd_type == 0x3) {
 		size_t i;
 
-		len = sprintf(out, "%d", vpd_type);
-		if (2 * vpd_len >= out_len - len) {
-			condlog(1, "%s: WWID overflow, type %d, %zu/%zu bytes required",
-				__func__, vpd_type,
-				2 * vpd_len + len + 1, out_len);
-			vpd_len = (out_len - len - 1) / 2;
-		}
+		if ((err = print_strbuf(&buf, "%d", vpd_type)) < 0)
+			return err;
 		for (i = 0; i < vpd_len; i++)
-			len += sprintf(out + len,
-				       "%02x", vpd[i]);
+			if ((err = print_strbuf(&buf, "%02x", vpd[i])) < 0)
+				return err;
 	} else if (vpd_type == 0x8) {
+		char type;
+
 		if (!memcmp("eui.", vpd, 4))
-			out[0] =  '2';
+			type =  '2';
 		else if (!memcmp("naa.", vpd, 4))
-			out[0] = '3';
+			type = '3';
 		else
-			out[0] = '8';
+			type = '8';
+		if ((err = fill_strbuf(&buf, type, 1)) < 0)
+			return err;
 
 		vpd += 4;
 		len = vpd_len - 4;
-		while (len > 2 && vpd[len - 2] == '\0')
-			--len;
-		if (len > out_len - 1) {
-			condlog(1, "%s: WWID overflow, type 8/%c, %zu/%zu bytes required",
-				__func__, out[0], len + 1, out_len);
-			len = out_len - 1;
+		if ((err = __append_strbuf_str(&buf, (const char *)vpd, len)) < 0)
+			return err;
+
+		/* The input is 0-padded, make sure the length is correct */
+		truncate_strbuf(&buf, strlen(get_strbuf_str(&buf)));
+		len = get_strbuf_len(&buf);
+		if (type != '8') {
+			char *buffer = __get_strbuf_buf(&buf);
+
+			for (i = 0; i < len; ++i)
+				buffer[i] = tolower(buffer[i]);
 		}
 
-		if (out[0] == '8')
-			for (i = 0; i < len; ++i)
-				out[1 + i] = vpd[i];
-		else
-			for (i = 0; i < len; ++i)
-				out[1 + i] = tolower(vpd[i]);
-
-		/* designator should be 0-terminated, but let's make sure */
-		out[len] = '\0';
-
 	} else if (vpd_type == 0x1) {
 		const unsigned char *p;
 		size_t p_len;
 
-		out[0] = '1';
-		len = 1;
-		while ((p = memchr(vpd, ' ', vpd_len))) {
+		if ((err = fill_strbuf(&buf, '1', 1)) < 0)
+			return err;
+		while (vpd && (p = memchr(vpd, ' ', vpd_len))) {
 			p_len = p - vpd;
-			if (len + p_len > out_len - 1) {
-				condlog(1, "%s: WWID overflow, type 1, %zu/%zu bytes required",
-					__func__, len + p_len, out_len);
-				p_len = out_len - len - 1;
-			}
-			memcpy(out + len, vpd, p_len);
-			len += p_len;
-			if (len >= out_len - 1) {
-				out[len] = '\0';
-				break;
-			}
-			out[len] = '_';
-			len ++;
-			if (len >= out_len - 1) {
-				out[len] = '\0';
-				break;
-			}
+			if ((err = __append_strbuf_str(&buf, (const char *)vpd,
+						       p_len)) < 0)
+				return err;
 			vpd = p;
 			vpd_len -= p_len;
-			while (vpd && *vpd == ' ') {
+			while (vpd && vpd_len > 0 && *vpd == ' ') {
 				vpd++;
 				vpd_len --;
 			}
+			if (vpd_len > 0 && (err = fill_strbuf(&buf, '_', 1)) < 0)
+				return err;
 		}
-		p_len = vpd_len;
-		if (p_len > 0 && len < out_len - 1) {
-			if (len + p_len > out_len - 1) {
-				condlog(1, "%s: WWID overflow, type 1, %zu/%zu bytes required",
-					__func__, len + p_len + 1, out_len);
-				p_len = out_len - len - 1;
-			}
-			memcpy(out + len, vpd, p_len);
-			len += p_len;
-			out[len] = '\0';
-		}
-		if (len > 1 && out[len - 1] == '_') {
-			out[len - 1] = '\0';
-			len--;
+		if (vpd_len > 0) {
+			if ((err = __append_strbuf_str(&buf, (const char *)vpd,
+						       vpd_len)) < 0)
+				return err;
 		}
 	}
+
+	len = get_strbuf_len(&buf);
+	if (len >= out_len) {
+		condlog(1, "%s: WWID overflow, type %d, %zu/%zu bytes required",
+			__func__, vpd_type, len, out_len);
+		if (vpd_type == 2 || vpd_type == 3)
+			/* designator must have an even number of characters */
+			len = 2 * (out_len / 2) - 1;
+		else
+			len = out_len - 1;
+	}
+	strlcpy(out, get_strbuf_str(&buf), len + 1);
 	return len;
 }
 
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 07/21] libmultipath: use strbuf in parse_vpd_pg83()

[Benjamin Marzinski]

On Wed, Dec 01, 2021 at 01:36:36PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> By using the strbuf API, the code gets more readable, as we need
> less output size checks.
> 
> While at it, avoid a possible crash by passing a NULL pointer
> to memchr().
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/discovery.c | 111 +++++++++++++++++----------------------
>  1 file changed, 48 insertions(+), 63 deletions(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 977aed9..7d939ae 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1121,6 +1121,7 @@ parse_vpd_pg83(const unsigned char *in, size_t in_len,
>  	size_t len, vpd_len, i;
>  	int vpd_type, prio = -1;
>  	int err = -ENODATA;
> +	STRBUF_ON_STACK(buf);
>  
>  	/* Need space at least for one digit */
>  	if (out_len <= 1)
> @@ -1239,92 +1240,76 @@ parse_vpd_pg83(const unsigned char *in, size_t in_len,
>  	if (vpd_type == 0x2 || vpd_type == 0x3) {
>  		size_t i;
>  
> -		len = sprintf(out, "%d", vpd_type);
> -		if (2 * vpd_len >= out_len - len) {
> -			condlog(1, "%s: WWID overflow, type %d, %zu/%zu bytes required",
> -				__func__, vpd_type,
> -				2 * vpd_len + len + 1, out_len);
> -			vpd_len = (out_len - len - 1) / 2;
> -		}
> +		if ((err = print_strbuf(&buf, "%d", vpd_type)) < 0)
> +			return err;
>  		for (i = 0; i < vpd_len; i++)
> -			len += sprintf(out + len,
> -				       "%02x", vpd[i]);
> +			if ((err = print_strbuf(&buf, "%02x", vpd[i])) < 0)
> +				return err;
>  	} else if (vpd_type == 0x8) {
> +		char type;
> +
>  		if (!memcmp("eui.", vpd, 4))
> -			out[0] =  '2';
> +			type =  '2';
>  		else if (!memcmp("naa.", vpd, 4))
> -			out[0] = '3';
> +			type = '3';
>  		else
> -			out[0] = '8';
> +			type = '8';
> +		if ((err = fill_strbuf(&buf, type, 1)) < 0)
> +			return err;
>  
>  		vpd += 4;
>  		len = vpd_len - 4;
> -		while (len > 2 && vpd[len - 2] == '\0')
> -			--len;
> -		if (len > out_len - 1) {
> -			condlog(1, "%s: WWID overflow, type 8/%c, %zu/%zu bytes required",
> -				__func__, out[0], len + 1, out_len);
> -			len = out_len - 1;
> +		if ((err = __append_strbuf_str(&buf, (const char *)vpd, len)) < 0)
> +			return err;
> +
> +		/* The input is 0-padded, make sure the length is correct */
> +		truncate_strbuf(&buf, strlen(get_strbuf_str(&buf)));
> +		len = get_strbuf_len(&buf);
> +		if (type != '8') {
> +			char *buffer = __get_strbuf_buf(&buf);
> +
> +			for (i = 0; i < len; ++i)
> +				buffer[i] = tolower(buffer[i]);
>  		}
>  
> -		if (out[0] == '8')
> -			for (i = 0; i < len; ++i)
> -				out[1 + i] = vpd[i];
> -		else
> -			for (i = 0; i < len; ++i)
> -				out[1 + i] = tolower(vpd[i]);
> -
> -		/* designator should be 0-terminated, but let's make sure */
> -		out[len] = '\0';
> -
>  	} else if (vpd_type == 0x1) {
>  		const unsigned char *p;
>  		size_t p_len;
>  
> -		out[0] = '1';
> -		len = 1;
> -		while ((p = memchr(vpd, ' ', vpd_len))) {
> +		if ((err = fill_strbuf(&buf, '1', 1)) < 0)
> +			return err;
> +		while (vpd && (p = memchr(vpd, ' ', vpd_len))) {
>  			p_len = p - vpd;
> -			if (len + p_len > out_len - 1) {
> -				condlog(1, "%s: WWID overflow, type 1, %zu/%zu bytes required",
> -					__func__, len + p_len, out_len);
> -				p_len = out_len - len - 1;
> -			}
> -			memcpy(out + len, vpd, p_len);
> -			len += p_len;
> -			if (len >= out_len - 1) {
> -				out[len] = '\0';
> -				break;
> -			}
> -			out[len] = '_';
> -			len ++;
> -			if (len >= out_len - 1) {
> -				out[len] = '\0';
> -				break;
> -			}
> +			if ((err = __append_strbuf_str(&buf, (const char *)vpd,
> +						       p_len)) < 0)
> +				return err;
>  			vpd = p;
>  			vpd_len -= p_len;
> -			while (vpd && *vpd == ' ') {
> +			while (vpd && vpd_len > 0 && *vpd == ' ') {
>  				vpd++;
>  				vpd_len --;
>  			}
> +			if (vpd_len > 0 && (err = fill_strbuf(&buf, '_', 1)) < 0)
> +				return err;
>  		}
> -		p_len = vpd_len;
> -		if (p_len > 0 && len < out_len - 1) {
> -			if (len + p_len > out_len - 1) {
> -				condlog(1, "%s: WWID overflow, type 1, %zu/%zu bytes required",
> -					__func__, len + p_len + 1, out_len);
> -				p_len = out_len - len - 1;
> -			}
> -			memcpy(out + len, vpd, p_len);
> -			len += p_len;
> -			out[len] = '\0';
> -		}
> -		if (len > 1 && out[len - 1] == '_') {
> -			out[len - 1] = '\0';
> -			len--;
> +		if (vpd_len > 0) {
> +			if ((err = __append_strbuf_str(&buf, (const char *)vpd,
> +						       vpd_len)) < 0)
> +				return err;
>  		}
>  	}
> +
> +	len = get_strbuf_len(&buf);
> +	if (len >= out_len) {
> +		condlog(1, "%s: WWID overflow, type %d, %zu/%zu bytes required",
> +			__func__, vpd_type, len, out_len);
> +		if (vpd_type == 2 || vpd_type == 3)
> +			/* designator must have an even number of characters */
> +			len = 2 * (out_len / 2) - 1;
> +		else
> +			len = out_len - 1;
> +	}
> +	strlcpy(out, get_strbuf_str(&buf), len + 1);
>  	return len;
>  }
>  
> -- 
> 2.33.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 08/21] libmultipath (coverity): fix tainted values in alua_rtpg.c

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Coverity needs tainted values limited by constant expressions.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/prioritizers/alua_rtpg.c | 13 ++++----
 libmultipath/prioritizers/alua_spc3.h | 43 +++++++++++++++++++++++----
 2 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/libmultipath/prioritizers/alua_rtpg.c b/libmultipath/prioritizers/alua_rtpg.c
index 420a2e3..3f9c0e7 100644
--- a/libmultipath/prioritizers/alua_rtpg.c
+++ b/libmultipath/prioritizers/alua_rtpg.c
@@ -27,7 +27,6 @@
 #include "../structs.h"
 #include "../prio.h"
 #include "../discovery.h"
-#include "../unaligned.h"
 #include "../debug.h"
 #include "alua_rtpg.h"
 
@@ -252,12 +251,12 @@ int
 get_target_port_group(const struct path * pp, unsigned int timeout)
 {
 	unsigned char		*buf;
-	struct vpd83_data *	vpd83;
-	struct vpd83_dscr *	dscr;
+	const struct vpd83_data *	vpd83;
+	const struct vpd83_dscr *	dscr;
 	int			rc;
 	int			buflen, scsi_buflen;
 
-	buflen = 4096;
+	buflen = VPD_BUFLEN;
 	buf = (unsigned char *)malloc(buflen);
 	if (!buf) {
 		PRINT_DEBUG("malloc failed: could not allocate"
@@ -298,13 +297,13 @@ get_target_port_group(const struct path * pp, unsigned int timeout)
 	rc = -RTPG_NO_TPG_IDENTIFIER;
 	FOR_EACH_VPD83_DSCR(vpd83, dscr) {
 		if (vpd83_dscr_istype(dscr, IDTYPE_TARGET_PORT_GROUP)) {
-			struct vpd83_tpg_dscr *p;
+			const struct vpd83_tpg_dscr *p;
 			if (rc != -RTPG_NO_TPG_IDENTIFIER) {
 				PRINT_DEBUG("get_target_port_group: more "
 					    "than one TPG identifier found!");
 				continue;
 			}
-			p  = (struct vpd83_tpg_dscr *)dscr->data;
+			p  = (const struct vpd83_tpg_dscr *)dscr->data;
 			rc = get_unaligned_be16(p->tpg);
 		}
 	}
@@ -377,7 +376,7 @@ get_asymmetric_access_state(const struct path *pp, unsigned int tpg,
 	uint64_t		scsi_buflen;
 	int fd = pp->fd;
 
-	buflen = 4096;
+	buflen = VPD_BUFLEN;
 	buf = (unsigned char *)malloc(buflen);
 	if (!buf) {
 		PRINT_DEBUG ("malloc failed: could not allocate"
diff --git a/libmultipath/prioritizers/alua_spc3.h b/libmultipath/prioritizers/alua_spc3.h
index 7ba2cf4..f0a4bc4 100644
--- a/libmultipath/prioritizers/alua_spc3.h
+++ b/libmultipath/prioritizers/alua_spc3.h
@@ -14,6 +14,7 @@
  */
 #ifndef __SPC3_H__
 #define __SPC3_H__
+#include "../unaligned.h"
 
 /*=============================================================================
  * Definitions to support the standard inquiry command as defined in SPC-3.
@@ -177,7 +178,7 @@ struct vpd83_dscr {
 } __attribute__((packed));
 
 static inline int
-vpd83_dscr_istype(struct vpd83_dscr *d, unsigned char type)
+vpd83_dscr_istype(const struct vpd83_dscr *d, unsigned char type)
 {
 	return ((d->b1 & 7) == type);
 }
@@ -190,6 +191,38 @@ struct vpd83_data {
 	struct vpd83_dscr	data[0];
 } __attribute__((packed));
 
+#define VPD_BUFLEN 4096
+
+/* Returns the max byte offset in the VPD page from the start of the page */
+static inline unsigned int vpd83_max_offs(const struct vpd83_data *p)
+{
+	uint16_t len = get_unaligned_be16(p->length) + 4;
+
+	return len <= VPD_BUFLEN ? len : VPD_BUFLEN;
+}
+
+static inline bool
+vpd83_descr_fits(const struct vpd83_dscr *d, const struct vpd83_data *p)
+{
+	ptrdiff_t max_offs = vpd83_max_offs(p);
+	ptrdiff_t offs = ((const char *)d - (const char *)p);
+
+	/* make sure we can read d->length */
+	if (offs < 0 || offs > max_offs - 4)
+		return false;
+
+	offs += d->length + 4;
+	return offs <= max_offs;
+}
+
+static inline const struct vpd83_dscr *
+vpd83_next_dscr(const struct vpd83_dscr *d, const struct vpd83_data *p)
+{
+	ptrdiff_t offs = ((const char *)d - (const char *)p) + d->length + 4;
+
+	return (const struct vpd83_dscr *)((const char *)p + offs);
+}
+
 /*-----------------------------------------------------------------------------
  * This macro should be used to walk through all identification descriptors
  * defined in the code page 0x83.
@@ -199,11 +232,9 @@ struct vpd83_data {
  */
 #define FOR_EACH_VPD83_DSCR(p, d) \
 		for( \
-			d = p->data; \
-			(((char *) d) - ((char *) p)) < \
-			get_unaligned_be16(p->length); \
-			d = (struct vpd83_dscr *) \
-				((char *) d + d->length + 4) \
+			d = p->data;		  \
+			vpd83_descr_fits(d, p);	  \
+			d = vpd83_next_dscr(d, p) \
 		)
 
 /*=============================================================================
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 08/21] libmultipath (coverity): fix tainted values in alua_rtpg.c

[Benjamin Marzinski]

On Wed, Dec 01, 2021 at 01:36:37PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Coverity needs tainted values limited by constant expressions.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/prioritizers/alua_rtpg.c | 13 ++++----
>  libmultipath/prioritizers/alua_spc3.h | 43 +++++++++++++++++++++++----
>  2 files changed, 43 insertions(+), 13 deletions(-)
> 
> diff --git a/libmultipath/prioritizers/alua_rtpg.c b/libmultipath/prioritizers/alua_rtpg.c
> index 420a2e3..3f9c0e7 100644
> --- a/libmultipath/prioritizers/alua_rtpg.c
> +++ b/libmultipath/prioritizers/alua_rtpg.c
> @@ -27,7 +27,6 @@
>  #include "../structs.h"
>  #include "../prio.h"
>  #include "../discovery.h"
> -#include "../unaligned.h"
>  #include "../debug.h"
>  #include "alua_rtpg.h"
>  
> @@ -252,12 +251,12 @@ int
>  get_target_port_group(const struct path * pp, unsigned int timeout)
>  {
>  	unsigned char		*buf;
> -	struct vpd83_data *	vpd83;
> -	struct vpd83_dscr *	dscr;
> +	const struct vpd83_data *	vpd83;
> +	const struct vpd83_dscr *	dscr;
>  	int			rc;
>  	int			buflen, scsi_buflen;
>  
> -	buflen = 4096;
> +	buflen = VPD_BUFLEN;
>  	buf = (unsigned char *)malloc(buflen);
>  	if (!buf) {
>  		PRINT_DEBUG("malloc failed: could not allocate"
> @@ -298,13 +297,13 @@ get_target_port_group(const struct path * pp, unsigned int timeout)
>  	rc = -RTPG_NO_TPG_IDENTIFIER;
>  	FOR_EACH_VPD83_DSCR(vpd83, dscr) {
>  		if (vpd83_dscr_istype(dscr, IDTYPE_TARGET_PORT_GROUP)) {
> -			struct vpd83_tpg_dscr *p;
> +			const struct vpd83_tpg_dscr *p;
>  			if (rc != -RTPG_NO_TPG_IDENTIFIER) {
>  				PRINT_DEBUG("get_target_port_group: more "
>  					    "than one TPG identifier found!");
>  				continue;
>  			}
> -			p  = (struct vpd83_tpg_dscr *)dscr->data;
> +			p  = (const struct vpd83_tpg_dscr *)dscr->data;
>  			rc = get_unaligned_be16(p->tpg);
>  		}
>  	}
> @@ -377,7 +376,7 @@ get_asymmetric_access_state(const struct path *pp, unsigned int tpg,
>  	uint64_t		scsi_buflen;
>  	int fd = pp->fd;
>  
> -	buflen = 4096;
> +	buflen = VPD_BUFLEN;
>  	buf = (unsigned char *)malloc(buflen);
>  	if (!buf) {
>  		PRINT_DEBUG ("malloc failed: could not allocate"
> diff --git a/libmultipath/prioritizers/alua_spc3.h b/libmultipath/prioritizers/alua_spc3.h
> index 7ba2cf4..f0a4bc4 100644
> --- a/libmultipath/prioritizers/alua_spc3.h
> +++ b/libmultipath/prioritizers/alua_spc3.h
> @@ -14,6 +14,7 @@
>   */
>  #ifndef __SPC3_H__
>  #define __SPC3_H__
> +#include "../unaligned.h"
>  
>  /*=============================================================================
>   * Definitions to support the standard inquiry command as defined in SPC-3.
> @@ -177,7 +178,7 @@ struct vpd83_dscr {
>  } __attribute__((packed));
>  
>  static inline int
> -vpd83_dscr_istype(struct vpd83_dscr *d, unsigned char type)
> +vpd83_dscr_istype(const struct vpd83_dscr *d, unsigned char type)
>  {
>  	return ((d->b1 & 7) == type);
>  }
> @@ -190,6 +191,38 @@ struct vpd83_data {
>  	struct vpd83_dscr	data[0];
>  } __attribute__((packed));
>  
> +#define VPD_BUFLEN 4096
> +
> +/* Returns the max byte offset in the VPD page from the start of the page */
> +static inline unsigned int vpd83_max_offs(const struct vpd83_data *p)
> +{
> +	uint16_t len = get_unaligned_be16(p->length) + 4;
> +
> +	return len <= VPD_BUFLEN ? len : VPD_BUFLEN;
> +}
> +
> +static inline bool
> +vpd83_descr_fits(const struct vpd83_dscr *d, const struct vpd83_data *p)
> +{
> +	ptrdiff_t max_offs = vpd83_max_offs(p);
> +	ptrdiff_t offs = ((const char *)d - (const char *)p);
> +
> +	/* make sure we can read d->length */
> +	if (offs < 0 || offs > max_offs - 4)
> +		return false;
> +
> +	offs += d->length + 4;
> +	return offs <= max_offs;
> +}
> +
> +static inline const struct vpd83_dscr *
> +vpd83_next_dscr(const struct vpd83_dscr *d, const struct vpd83_data *p)
> +{
> +	ptrdiff_t offs = ((const char *)d - (const char *)p) + d->length + 4;
> +
> +	return (const struct vpd83_dscr *)((const char *)p + offs);
> +}
> +
>  /*-----------------------------------------------------------------------------
>   * This macro should be used to walk through all identification descriptors
>   * defined in the code page 0x83.
> @@ -199,11 +232,9 @@ struct vpd83_data {
>   */
>  #define FOR_EACH_VPD83_DSCR(p, d) \
>  		for( \
> -			d = p->data; \
> -			(((char *) d) - ((char *) p)) < \
> -			get_unaligned_be16(p->length); \
> -			d = (struct vpd83_dscr *) \
> -				((char *) d + d->length + 4) \
> +			d = p->data;		  \
> +			vpd83_descr_fits(d, p);	  \
> +			d = vpd83_next_dscr(d, p) \
>  		)
>  
>  /*=============================================================================
> -- 
> 2.33.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 09/21] multipath, multipathd: exit if bindings file is broken

[mwilck]

From: Martin Wilck <mwilck@suse.com>

If check_alias_settings() returns error, the bindings file is
inconsistent and proceeding is potentially dangerous. Abort.

Found by coverity.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/main.c  |  5 ++++-
 multipathd/main.c | 15 ++++++++++++---
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index cd234ac..2a42748 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -958,7 +958,10 @@ main (int argc, char *argv[])
 		exit(RTVL_FAIL);
 	}
 
-	check_alias_settings(conf);
+	if (check_alias_settings(conf)) {
+		fprintf(stderr, "fatal configuration error, aborting");
+		exit(RTVL_FAIL);
+	}
 
 	if (optind < argc) {
 		dev = calloc(1, FILE_NAME_SIZE);
diff --git a/multipathd/main.c b/multipathd/main.c
index 4c7bcf2..1be42c5 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2819,7 +2819,9 @@ reconfigure (struct vectors * vecs)
 	reset_checker_classes();
 	if (bindings_read_only)
 		conf->bindings_read_only = bindings_read_only;
-	check_alias_settings(conf);
+
+	if (check_alias_settings(conf))
+		return 1;
 
 	uxsock_timeout = conf->uxsock_timeout;
 
@@ -3332,15 +3334,22 @@ child (__attribute__((unused)) void *param)
 		if (state == DAEMON_SHUTDOWN)
 			break;
 		if (state == DAEMON_CONFIGURE) {
+			int rc = 0;
+
 			pthread_cleanup_push(cleanup_lock, &vecs->lock);
 			lock(&vecs->lock);
 			pthread_testcancel();
 			if (!need_to_delay_reconfig(vecs))
-				reconfigure(vecs);
+				rc = reconfigure(vecs);
 			else
 				enable_delayed_reconfig();
 			lock_cleanup_pop(vecs->lock);
-			post_config_state(DAEMON_IDLE);
+			if (!rc)
+				post_config_state(DAEMON_IDLE);
+			else {
+				condlog(0, "fatal error applying configuration - aborting");
+				exit_daemon();
+			}
 		}
 	}
 
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 10/21] libmultipath (coverity): silence unchecked return value warning

[mwilck]

From: Martin Wilck <mwilck@suse.com>

path_get_tpgs() returns pp->tpgs, which we do check.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/propsel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index f7b8119..a842fc3 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -524,7 +524,7 @@ int select_checker(struct config *conf, struct path *pp)
 			ckr_name = RDAC;
 			goto out;
 		}
-		path_get_tpgs(pp);
+		(void)path_get_tpgs(pp);
 		if (pp->tpgs != TPGS_NONE && pp->tpgs != TPGS_UNDEF) {
 			ckr_name = TUR;
 			goto out;
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 11/21] multipath: remove pointless code from getopt processing

[mwilck]

From: Martin Wilck <mwilck@suse.com>

getopt() never returns 1, and sizeof(optarg) is always sizeof(char *).

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/main.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index 2a42748..5711acf 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -841,11 +841,8 @@ main (int argc, char *argv[])
 		condlog(1, "failed to register cleanup handler for vecs: %m");
 	while ((arg = getopt(argc, argv, ":adDcChl::eFfM:v:p:b:BrR:itTquUwW")) != EOF ) {
 		switch(arg) {
-		case 1: printf("optarg : %s\n",optarg);
-			break;
 		case 'v':
-			if (sizeof(optarg) > sizeof(char *) ||
-			    !isdigit(optarg[0])) {
+			if (!isdigit(optarg[0])) {
 				usage (argv[0]);
 				exit(RTVL_FAIL);
 			}
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 12/21] libmultipath (coverity): set umask before mkstemp

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Coverity SECURE_TEMP
(https://scan4.coverity.com/doc/en/cov_checker_ref.html#static_checker_SECURE_TEMP).
multipathd sets this umask anyway, but multipath doesn't.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/alias.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/libmultipath/alias.c b/libmultipath/alias.c
index 6ae512c..87c33af 100644
--- a/libmultipath/alias.c
+++ b/libmultipath/alias.c
@@ -578,13 +578,17 @@ static int fix_bindings_file(const struct config *conf,
 	int rc;
 	long fd;
 	char tempname[PATH_MAX];
+	mode_t old_umask;
 
 	if (safe_sprintf(tempname, "%s.XXXXXX", conf->bindings_file))
 		return -1;
+	/* coverity: SECURE_TEMP */
+	old_umask = umask(0077);
 	if ((fd = mkstemp(tempname)) == -1) {
 		condlog(1, "%s: mkstemp: %m", __func__);
 		return -1;
 	}
+	umask(old_umask);
 	pthread_cleanup_push(close_fd, (void*)fd);
 	rc = write_bindings_file(bindings, fd);
 	pthread_cleanup_pop(1);
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 13/21] multipathd (coverity): simplify set_oom_adj()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Coverity discourages stat-before-open. So simplify this function.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 53 ++++++++++++++---------------------------------
 1 file changed, 15 insertions(+), 38 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 1be42c5..36cc76f 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2952,51 +2952,28 @@ setscheduler (void)
 	return;
 }
 
-static void
-set_oom_adj (void)
+static void set_oom_adj(void)
 {
-#ifdef OOM_SCORE_ADJ_MIN
-	int retry = 1;
-	char *file = "/proc/self/oom_score_adj";
-	int score = OOM_SCORE_ADJ_MIN;
-#else
-	int retry = 0;
-	char *file = "/proc/self/oom_adj";
-	int score = OOM_ADJUST_MIN;
-#endif
 	FILE *fp;
-	struct stat st;
-	char *envp;
 
-	envp = getenv("OOMScoreAdjust");
-	if (envp) {
+	if (getenv("OOMScoreAdjust")) {
 		condlog(3, "Using systemd provided OOMScoreAdjust");
 		return;
 	}
-	do {
-		if (stat(file, &st) == 0){
-			fp = fopen(file, "w");
-			if (!fp) {
-				condlog(0, "couldn't fopen %s : %s", file,
-					strerror(errno));
-				return;
-			}
-			fprintf(fp, "%i", score);
-			fclose(fp);
-			return;
-		}
-		if (errno != ENOENT) {
-			condlog(0, "couldn't stat %s : %s", file,
-				strerror(errno));
-			return;
-		}
-#ifdef OOM_ADJUST_MIN
-		file = "/proc/self/oom_adj";
-		score = OOM_ADJUST_MIN;
-#else
-		retry = 0;
+#ifdef OOM_SCORE_ADJ_MIN
+	fp = fopen("/proc/self/oom_score_adj", "w");
+	if (fp) {
+		fprintf(fp, "%i", OOM_SCORE_ADJ_MIN);
+		fclose(fp);
+		return;
+	}
 #endif
-	} while (retry--);
+	fp = fopen("/proc/self/oom_adj", "w");
+	if (fp) {
+		fprintf(fp, "%i", OOM_ADJUST_MIN);
+		fclose(fp);
+		return;
+	}
 	condlog(0, "couldn't adjust oom score");
 }
 
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 14/21] kpartx: open /dev/loop-control only once

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Opening the same file repeatedly in a loop seems wrong.

For unknown reason, this patch caused gcc to diagnose a possible buffer
overflow for the device name, and I had to increase the buffer by
one byte.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/lopart.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/kpartx/lopart.c b/kpartx/lopart.c
index 9b65255..2661940 100644
--- a/kpartx/lopart.c
+++ b/kpartx/lopart.c
@@ -159,26 +159,28 @@ char *find_loop_by_file(const char *filename)
 
 char *find_unused_loop_device(void)
 {
-	char dev[20], *next_loop_dev = NULL;
+	char dev[21], *next_loop_dev = NULL;
 	int fd, next_loop = 0, somedev = 0, someloop = 0, loop_known = 0;
+	int next_loop_fd;
 	struct stat statbuf;
 	struct loop_info loopinfo;
 	FILE *procdev;
 
+	next_loop_fd = open("/dev/loop-control", O_RDWR);
+	if (next_loop_fd < 0)
+		return NULL;
+
+	if (!(fstat(next_loop_fd, &statbuf) == 0 && S_ISCHR(statbuf.st_mode))) {
+		close(next_loop_fd);
+		return NULL;
+	}
+
 	while (next_loop_dev == NULL) {
-		if (stat("/dev/loop-control", &statbuf) == 0 &&
-		    S_ISCHR(statbuf.st_mode)) {
-			int next_loop_fd;
-
-			next_loop_fd = open("/dev/loop-control", O_RDWR);
-			if (next_loop_fd < 0)
-				return NULL;
-			next_loop = ioctl(next_loop_fd, LOOP_CTL_GET_FREE);
+		next_loop = ioctl(next_loop_fd, LOOP_CTL_GET_FREE);
+		if (next_loop < 0) {
 			close(next_loop_fd);
-			if (next_loop < 0)
-				return NULL;
+			return NULL;
 		}
-
 		sprintf(dev, "/dev/loop%d", next_loop);
 
 		fd = open (dev, O_RDONLY);
@@ -199,6 +201,9 @@ char *find_unused_loop_device(void)
 		}
 		break;
 	}
+
+	close(next_loop_fd);
+
 	if (next_loop_dev)
 		return next_loop_dev;
 
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 15/21] kpartx: use opened loop device immediately

[mwilck]

From: Martin Wilck <mwilck@suse.com>

The code in find_unused_loop_device() goes through circles to
get an unused device, but it takes no care not to race with a different
process opening the same loop device. Use the once-opened
loop device for setup immediately instead of closing and re-opening it.

While at it, simplify the code somewhat.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/kpartx.c |  4 +--
 kpartx/lopart.c | 83 ++++++++++++++++++++-----------------------------
 kpartx/lopart.h |  3 +-
 3 files changed, 35 insertions(+), 55 deletions(-)

diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 7bc6454..3c49999 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -359,9 +359,7 @@ main(int argc, char **argv){
 			exit (0);
 
 		if (!loopdev) {
-			loopdev = find_unused_loop_device();
-
-			if (set_loop(loopdev, rpath, 0, &ro)) {
+			if (set_loop(&loopdev, rpath, 0, &ro)) {
 				fprintf(stderr, "can't set up loop\n");
 				exit (1);
 			}
diff --git a/kpartx/lopart.c b/kpartx/lopart.c
index 2661940..7041ddf 100644
--- a/kpartx/lopart.c
+++ b/kpartx/lopart.c
@@ -39,24 +39,6 @@
 #define LOOP_CTL_GET_FREE       0x4C82
 #endif
 
-static char *
-xstrdup (const char *s)
-{
-	char *t;
-
-	if (s == NULL)
-		return NULL;
-
-	t = strdup (s);
-
-	if (t == NULL) {
-		fprintf(stderr, "not enough memory");
-		exit(1);
-	}
-
-	return t;
-}
-
 #define SIZE(a) (sizeof(a)/sizeof(a[0]))
 
 char *find_loop_by_file(const char *filename)
@@ -157,9 +139,9 @@ char *find_loop_by_file(const char *filename)
 	return found;
 }
 
-char *find_unused_loop_device(void)
+static char *find_unused_loop_device(int mode, int *loop_fd)
 {
-	char dev[21], *next_loop_dev = NULL;
+	char dev[21];
 	int fd, next_loop = 0, somedev = 0, someloop = 0, loop_known = 0;
 	int next_loop_fd;
 	struct stat statbuf;
@@ -168,45 +150,48 @@ char *find_unused_loop_device(void)
 
 	next_loop_fd = open("/dev/loop-control", O_RDWR);
 	if (next_loop_fd < 0)
-		return NULL;
+		goto no_loop_fd;
 
-	if (!(fstat(next_loop_fd, &statbuf) == 0 && S_ISCHR(statbuf.st_mode))) {
-		close(next_loop_fd);
-		return NULL;
-	}
+	if (!(fstat(next_loop_fd, &statbuf) == 0 && S_ISCHR(statbuf.st_mode)))
+		goto nothing_found;
 
-	while (next_loop_dev == NULL) {
+	for (;;) {
 		next_loop = ioctl(next_loop_fd, LOOP_CTL_GET_FREE);
-		if (next_loop < 0) {
-			close(next_loop_fd);
-			return NULL;
-		}
+		if (next_loop < 0)
+			goto nothing_found;
+
 		sprintf(dev, "/dev/loop%d", next_loop);
 
-		fd = open (dev, O_RDONLY);
+		fd = open (dev, mode);
 		if (fd >= 0) {
 			if (fstat (fd, &statbuf) == 0 &&
 			    S_ISBLK(statbuf.st_mode)) {
 				somedev++;
 				if(ioctl (fd, LOOP_GET_STATUS, &loopinfo) == 0)
 					someloop++;		/* in use */
-				else if (errno == ENXIO)
-					next_loop_dev = xstrdup(dev);
+				else if (errno == ENXIO) {
+					char *name = strdup(dev);
+
+					if (name == NULL)
+						close(fd);
+					else
+						*loop_fd = fd;
+					close(next_loop_fd);
+					return name;
+				}
 
 			}
 			close (fd);
 
 			/* continue trying as long as devices exist */
-			continue;
-		}
-		break;
+		} else
+			break;
 	}
 
+nothing_found:
 	close(next_loop_fd);
 
-	if (next_loop_dev)
-		return next_loop_dev;
-
+no_loop_fd:
 	/* Nothing found. Why not? */
 	if ((procdev = fopen(PROC_DEVICES, "r")) != NULL) {
 		char line[100];
@@ -248,10 +233,10 @@ char *find_unused_loop_device(void)
 	return NULL;
 }
 
-int set_loop(const char *device, const char *file, int offset, int *loopro)
+int set_loop(char **device, const char *file, int offset, int *loopro)
 {
 	struct loop_info loopinfo;
-	int fd, ffd, mode;
+	int fd = -1, ret = 1, ffd, mode;
 
 	mode = (*loopro ? O_RDONLY : O_RDWR);
 
@@ -266,9 +251,9 @@ int set_loop(const char *device, const char *file, int offset, int *loopro)
 		}
 	}
 
-	if ((fd = open (device, mode)) < 0) {
+	*device = find_unused_loop_device(mode, &fd);
+	if (!*device) {
 		close(ffd);
-		perror (device);
 		return 1;
 	}
 
@@ -282,22 +267,20 @@ int set_loop(const char *device, const char *file, int offset, int *loopro)
 
 	if (ioctl(fd, LOOP_SET_FD, (void*)(uintptr_t)(ffd)) < 0) {
 		perror ("ioctl: LOOP_SET_FD");
-		close (fd);
-		close (ffd);
-		return 1;
+		goto out;
 	}
 
 	if (ioctl (fd, LOOP_SET_STATUS, &loopinfo) < 0) {
 		(void) ioctl (fd, LOOP_CLR_FD, 0);
 		perror ("ioctl: LOOP_SET_STATUS");
-		close (fd);
-		close (ffd);
-		return 1;
+		goto out;
 	}
+	ret = 0;
 
+out:
 	close (fd);
 	close (ffd);
-	return 0;
+	return ret;
 }
 
 int del_loop(const char *device)
diff --git a/kpartx/lopart.h b/kpartx/lopart.h
index d3bad10..c73ab23 100644
--- a/kpartx/lopart.h
+++ b/kpartx/lopart.h
@@ -1,5 +1,4 @@
 extern int verbose;
-extern int set_loop (const char *, const char *, int, int *);
+extern int set_loop (char **, const char *, int, int *);
 extern int del_loop (const char *);
-extern char * find_unused_loop_device (void);
 extern char * find_loop_by_file (const char *);
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 16/21] kpartx: find_unused_loop_device(): add newlines

[mwilck]

From: Martin Wilck <mwilck@suse.com>

... to avoid these messages being joined with the error message
from the caller.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/lopart.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kpartx/lopart.c b/kpartx/lopart.c
index 7041ddf..512a59f 100644
--- a/kpartx/lopart.c
+++ b/kpartx/lopart.c
@@ -210,26 +210,26 @@ no_loop_fd:
 	}
 
 	if (!somedev)
-		fprintf(stderr, "mount: could not find any device /dev/loop#");
+		fprintf(stderr, "mount: could not find any device /dev/loop#\n");
 
 	else if (!someloop) {
 		if (loop_known == 1)
 			fprintf(stderr,
 				"mount: Could not find any loop device.\n"
-				"       Maybe /dev/loop# has a wrong major number?");
+				"       Maybe /dev/loop# has a wrong major number?\n");
 		else if (loop_known == -1)
 			fprintf(stderr,
 				"mount: Could not find any loop device, and, according to %s,\n"
 				"       this kernel does not know about the loop device.\n"
-				"       (If so, then recompile or `modprobe loop'.)",
+				"       (If so, then recompile or `modprobe loop'.)\n",
 				PROC_DEVICES);
 		else
 			fprintf(stderr,
 				"mount: Could not find any loop device. Maybe this kernel does not know\n"
 				"       about the loop device (then recompile or `modprobe loop'), or\n"
-				"       maybe /dev/loop# has the wrong major number?");
+				"       maybe /dev/loop# has the wrong major number?\n");
 	} else
-		fprintf(stderr, "mount: could not find any free loop device");
+		fprintf(stderr, "mount: could not find any free loop device\n");
 	return NULL;
 }
 
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 17/21] multipathd (coverity): daemonize(): use dup2

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Modify the file descriptors atomically usign dup2(), and make
sure to cleanup properly even in case of an error, and to not
close stdout/in/err if the program had been started with any of
them closed.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 36cc76f..7a57a79 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3337,11 +3337,18 @@ failed:
 	return sd_notify_exit(exit_code);
 }
 
+static void cleanup_close(int *pfd)
+{
+	if (*pfd != -1 && *pfd != STDIN_FILENO && *pfd != STDOUT_FILENO &&
+	    *pfd != STDERR_FILENO)
+		close(*pfd);
+}
+
 static int
 daemonize(void)
 {
 	int pid;
-	int dev_null_fd;
+	int dev_null_fd __attribute__((cleanup(cleanup_close))) = -1;
 
 	if( (pid = fork()) < 0){
 		fprintf(stderr, "Failed first fork : %s\n", strerror(errno));
@@ -3367,25 +3374,21 @@ daemonize(void)
 		_exit(0);
 	}
 
-	close(STDIN_FILENO);
-	if (dup(dev_null_fd) < 0) {
-		fprintf(stderr, "cannot dup /dev/null to stdin : %s\n",
+	if (dup2(dev_null_fd, STDIN_FILENO) < 0) {
+		fprintf(stderr, "cannot dup2 /dev/null to stdin : %s\n",
 			strerror(errno));
 		_exit(0);
 	}
-	close(STDOUT_FILENO);
-	if (dup(dev_null_fd) < 0) {
-		fprintf(stderr, "cannot dup /dev/null to stdout : %s\n",
+	if (dup2(dev_null_fd, STDOUT_FILENO) < 0) {
+		fprintf(stderr, "cannot dup2 /dev/null to stdout : %s\n",
 			strerror(errno));
 		_exit(0);
 	}
-	close(STDERR_FILENO);
-	if (dup(dev_null_fd) < 0) {
+	if (dup2(dev_null_fd, STDERR_FILENO) < 0) {
 		fprintf(stderr, "cannot dup /dev/null to stderr : %s\n",
 			strerror(errno));
 		_exit(0);
 	}
-	close(dev_null_fd);
 	daemon_pid = getpid();
 	return 0;
 }
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 18/21] libmultipath (coverity): avoid sleeping in dm_mapname()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

dm_mapname is called while holding the vecs lock, and shouldn't
sleep. All callers handle a NULL return value.

This code dates back to the initial commit creating multipathd,
66b457b ("[multipathd]") from 2005. I am pretty certain that the comment
"device map might not be ready when we get here from uevent trigger"
doesn't hold any more (if it has ever been true). Perhaps the early
code acted on "add" events for maps, which would indeed not necessarily
have been set up. On the actually relevant "change" events, the map
name has to be set.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/devmapper.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index dd293aa..1dc83df 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -1389,7 +1389,6 @@ dm_mapname(int major, int minor)
 	const char *map;
 	struct dm_task *dmt;
 	int r;
-	int loop = MAX_WAIT * LOOPS_PER_SEC;
 
 	if (!(dmt = libmp_dm_task_create(DM_DEVICE_STATUS)))
 		return NULL;
@@ -1399,23 +1398,9 @@ dm_mapname(int major, int minor)
 		goto bad;
 
 	dm_task_no_open_count(dmt);
-
-	/*
-	 * device map might not be ready when we get here from
-	 * daemon uev_trigger -> uev_add_map
-	 */
-	while (--loop) {
-		r = libmp_dm_task_run(dmt);
-
-		if (r)
-			break;
-
-		usleep(1000 * 1000 / LOOPS_PER_SEC);
-	}
-
+	r = libmp_dm_task_run(dmt);
 	if (!r) {
 		dm_log_error(2, DM_DEVICE_STATUS, dmt);
-		condlog(0, "%i:%i: timeout fetching map name", major, minor);
 		goto bad;
 	}
 
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 19/21] libmultipath (coverity): Revert "setup_map: wait for pending path checkers to finish"

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This reverts commit adf551f601596aac0774884c8a5b6b0f74721cfc.

Since 2b1fe13 ("libmultipath: allow force reload with no active paths"),
reloading maps with pending paths is no issue any more. The path checker
is going to set the correct status of the path quickly. If the paths are
down, I/O will eventually fail.

Sleeping with the vecs lock held is undesirable though, so avoid waiting
in setup_map().

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 63 +---------------------------------------
 1 file changed, 1 insertion(+), 62 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 7f32da2..6745976 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -43,10 +43,6 @@
 #include "sysfs.h"
 #include "io_err_stat.h"
 
-/* Time in ms to wait for pending checkers in setup_map() */
-#define WAIT_CHECKERS_PENDING_MS 10
-#define WAIT_ALL_CHECKERS_PENDING_MS 90
-
 /* group paths in pg by host adapter
  */
 int group_by_host_adapter(struct pathgroup *pgp, vector adapters)
@@ -260,42 +256,11 @@ int rr_optimize_path_order(struct pathgroup *pgp)
 	return 0;
 }
 
-static int wait_for_pending_paths(struct multipath *mpp,
-				  struct config *conf,
-				  int n_pending, int goal, int wait_ms)
-{
-	static const struct timespec millisec =
-		{ .tv_sec = 0, .tv_nsec = 1000*1000 };
-	int i, j;
-	struct path *pp;
-	struct pathgroup *pgp;
-	struct timespec ts;
-
-	do {
-		vector_foreach_slot(mpp->pg, pgp, i) {
-			vector_foreach_slot(pgp->paths, pp, j) {
-				if (pp->state != PATH_PENDING)
-					continue;
-				pp->state = get_state(pp, conf,
-						      0, PATH_PENDING);
-				if (pp->state != PATH_PENDING &&
-				    --n_pending <= goal)
-					return 0;
-			}
-		}
-		ts = millisec;
-		while (nanosleep(&ts, &ts) != 0 && errno == EINTR)
-			/* nothing */;
-	} while (--wait_ms > 0);
-
-	return n_pending;
-}
-
 int setup_map(struct multipath *mpp, char **params, struct vectors *vecs)
 {
 	struct pathgroup * pgp;
 	struct config *conf;
-	int i, n_paths, marginal_pathgroups;
+	int i, marginal_pathgroups;
 	char *save_attr;
 
 	/*
@@ -394,7 +359,6 @@ int setup_map(struct multipath *mpp, char **params, struct vectors *vecs)
 	if (marginal_path_check_enabled(mpp))
 		start_io_err_stat_thread(vecs);
 
-	n_paths = VECTOR_SIZE(mpp->paths);
 	/*
 	 * assign paths to path groups -- start with no groups and all paths
 	 * in mpp->paths
@@ -409,31 +373,6 @@ int setup_map(struct multipath *mpp, char **params, struct vectors *vecs)
 	if (group_paths(mpp, marginal_pathgroups))
 		return 1;
 
-	/*
-	 * If async state detection is used, see if pending state checks
-	 * have finished, to get nr_active right. We can't wait until the
-	 * checkers time out, as that may take 30s or more, and we are
-	 * holding the vecs lock.
-	 */
-	if (conf->force_sync == 0 && n_paths > 0) {
-		int n_pending = pathcount(mpp, PATH_PENDING);
-
-		if (n_pending > 0)
-			n_pending = wait_for_pending_paths(
-				mpp, conf, n_pending, 0,
-				WAIT_CHECKERS_PENDING_MS);
-		/* ALL paths pending - wait some more, but be satisfied
-		   with only some paths finished */
-		if (n_pending == n_paths)
-			n_pending = wait_for_pending_paths(
-				mpp, conf, n_pending,
-				n_paths >= 4 ? 2 : 1,
-				WAIT_ALL_CHECKERS_PENDING_MS);
-		if (n_pending > 0)
-			condlog(2, "%s: setting up map with %d/%d path checkers pending",
-				mpp->alias, n_pending, n_paths);
-	}
-
 	/*
 	 * ponders each path group and determine highest prio pg
 	 * to switch over (default to first)
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 20/21] libmultipath (coverity): check return values in dm_get_multipath()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Coverity CID 374151.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/devmapper.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 1dc83df..c0eb335 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -1294,8 +1294,10 @@ struct multipath *dm_get_multipath(const char *name)
 	if (dm_get_map(name, &mpp->size, NULL) != DMP_OK)
 		goto out;
 
-	dm_get_uuid(name, mpp->wwid, WWID_SIZE);
-	dm_get_info(name, &mpp->dmi);
+	if (dm_get_uuid(name, mpp->wwid, WWID_SIZE) != 0)
+		condlog(2, "%s: failed to get uuid for %s", __func__, name);
+	if (dm_get_info(name, &mpp->dmi) != 0)
+		condlog(2, "%s: failed to get info for %s", __func__, name);
 
 	return mpp;
 out:
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 21/21] libmultipath: update_pathvec_from_dm: don't force DI_WWID

[mwilck]

From: Martin Wilck <mwilck@suse.com>

For existing paths, we don't need to force setting DI_WWID. It's
pointless because it would matter only in multipathd, and all
callers of update_multipath_table() / update_pathvec_from_dm()
in multipathd call it with flags=0, so this code won't be executed.
And this makes sense, because the path would either have been
cleanly initialized via path_discover() or an uevent, or pathinfo()
would have been called in update_pathvec_from_dm() in an earlier
invocation.

Also remove the misleading comment, and make coverity happy by
checking the return value of pathinfo().

---

Note: I haven't fully made up my mind whether we should act
on a pathinfo() failure here. For now, let the log message suffice.
Because the pathinfo flags are usually clear and pp->udev exists,
the only possible failure would be filtering by foreign libraries
or by devnode, which seems highly unlikely if it hasn't happened
beforehand.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs_vec.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index df5709a..bfec840 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -149,16 +149,15 @@ bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
 			 * uninitialized struct path to pgp->paths, with only
 			 * pp->dev_t filled in. Thus if pp->udev is set here,
 			 * we know that the path is in pathvec already.
-			 * However, it's possible that the path in pathvec is
-			 * different from the one the kernel still had in its
-			 * map.
 			 */
 			if (pp->udev) {
 				if (pathinfo_flags & ~DI_NOIO) {
 					conf = get_multipath_config();
 					pthread_cleanup_push(put_multipath_config,
 							     conf);
-					pathinfo(pp, conf, pathinfo_flags|DI_WWID);
+					if (pathinfo(pp, conf, pathinfo_flags) != PATHINFO_OK)
+						condlog(2, "%s: pathinfo failed for existing path %s (flags=0x%x)",
+							__func__, pp->dev, pathinfo_flags);
 					pthread_cleanup_pop(1);
 				}
 			} else {
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel