| Message ID | 20211118231338.22358-6-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | multipath-tools: coverity fixes | expand |
On Fri, Nov 19, 2021 at 12:13:22AM +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. > This looks fine, but I do wonder if we are being too strict. I'm not sure we should be failing, if sg_inq wouldn't fail. The goal of the fallback code is to get the WWID the udev would have gotten, and being lenient with scsi devices that don't quite follow the standard is usually the best policy. Thoughts? -Ben > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > libmultipath/discovery.c | 139 +++++++++++++++++++++++++-------------- > 1 file changed, 90 insertions(+), 49 deletions(-) > > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c > index 5edf959..5ce9031 100644 > --- a/libmultipath/discovery.c > +++ b/libmultipath/discovery.c > @@ -37,6 +37,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" }, > @@ -1085,6 +1087,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 > */ > @@ -1114,84 +1118,122 @@ 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; > > - d = in + 4; > - while (d < in + in_len) { > - /* Select 'association: LUN' */ > - if ((d[1] & 0x30) != 0) { > - d += d[3] + 4; > - continue; > - } > - switch (d[1] & 0xf) { > + /* Need space at least for one digit */ > + if (out_len <= 1) > + return 0; > + > + d = in + 4; > + while (d <= in + in_len - 4) { > + bool invalid = false; > + int new_prio = -1; > + > + /* Select 'association: LUN' */ > + 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; > + } > + 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]); > + err = -EINVAL; > + break; > + } > + 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; > > @@ -1205,10 +1247,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'; > @@ -1315,11 +1353,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; > } > @@ -1330,8 +1369,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); > @@ -1375,7 +1416,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) > @@ -1391,7 +1432,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
On Mon, 2021-11-29 at 19:17 -0600, Benjamin Marzinski wrote: > On Fri, Nov 19, 2021 at 12:13:22AM +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. > > > > This looks fine, but I do wonder if we are being too strict. I'm not > sure we should be failing, if sg_inq wouldn't fail. The goal of the > fallback code is to get the WWID the udev would have gotten, and > being > lenient with scsi devices that don't quite follow the standard is > usually the best policy. Thoughts? If we encounter a broken VPD entry, IMO we can't continue reading further entries, because if the entry is broken, we can't trust the length value which is part of the entry. We may be reading total junk if we follow it. We might simply discard the broken entry, stop iterating over the designators, see if we got a usable designator up to that point (i.e., previously), and if yes, use that entry, printing a big fat warning. That means we'd ultimately fail only if there was no usable designator before the broken one. Would you prefer that strategy? Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Tue, Nov 30, 2021 at 12:21:44PM +0100, Martin Wilck wrote: > On Mon, 2021-11-29 at 19:17 -0600, Benjamin Marzinski wrote: > > On Fri, Nov 19, 2021 at 12:13:22AM +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. > > > > > > > This looks fine, but I do wonder if we are being too strict. I'm not > > sure we should be failing, if sg_inq wouldn't fail. The goal of the > > fallback code is to get the WWID the udev would have gotten, and > > being > > lenient with scsi devices that don't quite follow the standard is > > usually the best policy. Thoughts? > > If we encounter a broken VPD entry, IMO we can't continue reading > further entries, because if the entry is broken, we can't trust the > length value which is part of the entry. We may be reading total junk > if we follow it. > > We might simply discard the broken entry, stop iterating over the > designators, see if we got a usable designator up to that point (i.e., > previously), and if yes, use that entry, printing a big fat warning. > That means we'd ultimately fail only if there was no usable designator > before the broken one. > > Would you prefer that strategy? If you export scsi IDs from sg_inq, it will print them out until it hits an error, so I would prefer if we used the best designator we got before we hit the error, to match it. -Ben > > Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Tue, 2021-11-30 at 11:30 -0600, Benjamin Marzinski wrote: > > If you export scsi IDs from sg_inq, it will print them out until it > hits an error, so I would prefer if we used the best designator we got > before we hit the error, to match it. That's what my code does (embarrassingly, I didn't realize that when I wrote my previous reply). In shortened pseudo code: int prio = -1; int err = -ENODATA; d = first_designator(); while (d <= in + in_len - 4) { bool invalid = false; int new_prio = -1; if (designator_is_invalid(d)) invalid = true; else if (lun_association(d)) new_prio = prio(d); next_designator: if (invalid) { err = -EINVAL; break; /** see below **/ } if (new_prio > prio) { vpd = d; prio = new_prio; } d = next_designator(d); } /* if we did find a valid designator, prio will be > 0, and we will not error out */ if (prio <= 0) return err; convert_designator_to_wwid(d); If you think we should use a different strategy, please explain. We *could* try to go on even after encountering broken designators, assuming the length field is correct if it's within the given limits, even if the rest is bogus. So baiscally instead of the break statement above, we'd continue the loop. Would you prefer that? Regards, Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Tue, Nov 30, 2021 at 09:14:10PM +0100, Martin Wilck wrote: > On Tue, 2021-11-30 at 11:30 -0600, Benjamin Marzinski wrote: > > > > If you export scsi IDs from sg_inq, it will print them out until it > > hits an error, so I would prefer if we used the best designator we got > > before we hit the error, to match it. > > That's what my code does (embarrassingly, I didn't realize that when I > wrote my previous reply). > > In shortened pseudo code: > > int prio = -1; > int err = -ENODATA; > > d = first_designator(); > while (d <= in + in_len - 4) { > bool invalid = false; > int new_prio = -1; > > if (designator_is_invalid(d)) > invalid = true; > else if (lun_association(d)) > new_prio = prio(d); > > next_designator: > if (invalid) { > err = -EINVAL; > break; /** see below **/ > } > if (new_prio > prio) { > vpd = d; > prio = new_prio; > } > d = next_designator(d); > } > > /* if we did find a valid designator, prio will be > 0, and we > will not error out */ > if (prio <= 0) > return err; > > convert_designator_to_wwid(d); > > If you think we should use a different strategy, please explain. > We *could* try to go on even after encountering broken designators, > assuming the length field is correct if it's within the given limits, > even if the rest is bogus. So baiscally instead of the break statement > above, we'd continue the loop. > > Would you prefer that? Again looking at sg_inq, it will loop through all the descriptors, trusting the length field, until it gets to the whole data length. It prints a warning if it doesn't end up at exactly the data length, but still exports all the IDs it finds. If an individual descriptor doesn't make sense, it gets skipped. That would be my preference. -Ben > Regards, > Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Tue, 2021-11-30 at 15:14 -0600, Benjamin Marzinski wrote: > > > > If you think we should use a different strategy, please explain. > > We *could* try to go on even after encountering broken designators, > > assuming the length field is correct if it's within the given > > limits, > > even if the rest is bogus. So baiscally instead of the break > > statement > > above, we'd continue the loop. > > > > Would you prefer that? > > Again looking at sg_inq, it will loop through all the descriptors, > trusting the length field, until it gets to the whole data length. It > prints a warning if it doesn't end up at exactly the data length, but > still exports all the IDs it finds. If an individual descriptor > doesn't > make sense, it gets skipped. That would be my preference. Ok, I'll try to fix it up. Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c index 5edf959..5ce9031 100644 --- a/libmultipath/discovery.c +++ b/libmultipath/discovery.c @@ -37,6 +37,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" }, @@ -1085,6 +1087,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 */ @@ -1114,84 +1118,122 @@ 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; - d = in + 4; - while (d < in + in_len) { - /* Select 'association: LUN' */ - if ((d[1] & 0x30) != 0) { - d += d[3] + 4; - continue; - } - switch (d[1] & 0xf) { + /* Need space at least for one digit */ + if (out_len <= 1) + return 0; + + d = in + 4; + while (d <= in + in_len - 4) { + bool invalid = false; + int new_prio = -1; + + /* Select 'association: LUN' */ + 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; + } + 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]); + err = -EINVAL; + break; + } + 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; @@ -1205,10 +1247,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'; @@ -1315,11 +1353,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; } @@ -1330,8 +1369,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); @@ -1375,7 +1416,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) @@ -1391,7 +1432,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)