diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index 8856bca459..4c96bc1a06 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -466,8 +466,12 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re bool hasChecksum = false; bool hasRW = false; + bool endReached = false; - while (fieldPos + 3 < resPos + resDataLen) { + /* Note the equal sign - fields may have a zero length in which case they will + * just occupy 3 header bytes. In the in case of the RW field this may mean that + * no more space is left in the section. */ + while (fieldPos + 3 <= resPos + resDataLen) { /* Keyword resources consist of keywords (2 ASCII bytes per the spec) and 1-byte length. */ if (virPCIVPDReadVPDBytes(vpdFileFd, buf, 3, fieldPos, csum) != 3) { /* Invalid field encountered which means the resource itself is invalid too. Report @@ -518,6 +522,13 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re return false; } + if (resPos + resDataLen < fieldPos + fieldDataLen) { + /* In this case the field cannot simply be skipped since the position of the + * next field is determined based on the length of a previous field. */ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("A field data length violates the resource length boundary.")); + return false; + } if (virPCIVPDReadVPDBytes(vpdFileFd, buf, bytesToRead, fieldPos, csum) != bytesToRead) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not parse a resource field data - VPD has invalid format")); @@ -546,12 +557,13 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re hasChecksum = true; g_free(g_steal_pointer(&fieldKeyword)); g_free(g_steal_pointer(&fieldValue)); - continue; + break; } else if (fieldFormat == VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RDWR) { /* Skip the read-write space since it is used for indication only. */ hasRW = true; g_free(g_steal_pointer(&fieldKeyword)); g_free(g_steal_pointer(&fieldValue)); + break; } else if (fieldFormat == VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST) { /* Skip unknown fields */ g_free(g_steal_pointer(&fieldKeyword)); @@ -579,14 +591,25 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re g_free(g_steal_pointer(&fieldKeyword)); g_free(g_steal_pointer(&fieldValue)); } - if (readOnly && !hasChecksum) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("VPD-R does not contain the mandatory RV field")); - return false; - } else if (!readOnly && !hasRW) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("VPD-W does not contain the mandatory RW field")); + + /* May have exited the loop prematurely in case RV or RW were encountered and + * they were not the last fields in the section. */ + endReached = (fieldPos >= resPos + resDataLen); + if (readOnly && !(hasChecksum && endReached)) { + VIR_DEBUG("VPD-R does not contain the mandatory RV field as the last field"); return false; + } else if (!readOnly && !endReached) { + /* The lack of RW is allowed on purpose in the read-write section since some vendors + * violate the PCI/PCIe specs and do not include it, however, this does not prevent parsing + * of valid data. If the RW is present, however, we make sure it is the last field in + * the read-write section. */ + if (hasRW) { + VIR_DEBUG("VPD-W section parsing ended prematurely (RW is not the last field)."); + return false; + } else { + VIR_DEBUG("VPD-W section parsing ended prematurely."); + return false; + } } return true; diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index 2cc9069132..a99bde2b92 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -597,6 +597,107 @@ testVirPCIVPDParseFullVPD(const void *opaque G_GNUC_UNUSED) return ret; } +static int +testVirPCIVPDParseZeroLengthRW(const void *opaque G_GNUC_UNUSED) +{ + int fd = -1; + size_t dataLen = 0; + + g_autoptr(virPCIVPDResource) res = NULL; + virPCIVPDResourceCustom *custom = NULL; + + /* The RW field has a zero length which means there is no more RW space left. */ + const uint8_t fullVPDExample[] = { + VPD_STRING_RESOURCE_EXAMPLE_HEADER, VPD_STRING_RESOURCE_EXAMPLE_DATA, + VPD_R_FIELDS_EXAMPLE_HEADER, VPD_R_FIELDS_EXAMPLE_DATA, + PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_READ_WRITE_LARGE_RESOURCE_FLAG, 0x08, 0x00, + 'V', 'Z', 0x02, '4', '2', + 'R', 'W', 0x00, + PCI_VPD_RESOURCE_END_VAL + }; + + dataLen = sizeof(fullVPDExample) / sizeof(uint8_t); + fd = virCreateAnonymousFile(fullVPDExample, dataLen); + res = virPCIVPDParse(fd); + VIR_FORCE_CLOSE(fd); + + if (!res) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "The resource pointer is NULL after parsing which is unexpected"); + return -1; + } + + if (!res->ro) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "Read-only keywords are missing from the VPD resource."); + return -1; + } else if (!res->rw) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "Read-write keywords are missing from the VPD resource."); + return -1; + } + + if (testVirPCIVPDValidateExampleReadOnlyFields(res)) + return -1; + + custom = g_ptr_array_index(res->rw->vendor_specific, 0); + if (custom->idx != 'Z' || STRNEQ_NULLABLE(custom->value, "42")) + return -1; + + custom = NULL; + return 0; +} + +static int +testVirPCIVPDParseNoRW(const void *opaque G_GNUC_UNUSED) +{ + int fd = -1; + size_t dataLen = 0; + + g_autoptr(virPCIVPDResource) res = NULL; + virPCIVPDResourceCustom *custom = NULL; + + /* The RW field has a zero length which means there is no more RW space left. */ + const uint8_t fullVPDExample[] = { + VPD_STRING_RESOURCE_EXAMPLE_HEADER, VPD_STRING_RESOURCE_EXAMPLE_DATA, + VPD_R_FIELDS_EXAMPLE_HEADER, VPD_R_FIELDS_EXAMPLE_DATA, + PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_READ_WRITE_LARGE_RESOURCE_FLAG, 0x05, 0x00, + 'V', 'Z', 0x02, '4', '2', + PCI_VPD_RESOURCE_END_VAL + }; + + dataLen = sizeof(fullVPDExample) / sizeof(uint8_t); + fd = virCreateAnonymousFile(fullVPDExample, dataLen); + res = virPCIVPDParse(fd); + VIR_FORCE_CLOSE(fd); + + if (!res) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "The resource pointer is NULL after parsing which is unexpected"); + return -1; + } + + if (!res->ro) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "Read-only keywords are missing from the VPD resource."); + return -1; + } else if (!res->rw) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "Read-write keywords are missing from the VPD resource."); + return -1; + } + + if (testVirPCIVPDValidateExampleReadOnlyFields(res)) + return -1; + + custom = g_ptr_array_index(res->rw->vendor_specific, 0); + if (custom->idx != 'Z' || STRNEQ_NULLABLE(custom->value, "42")) + return -1; + + custom = NULL; + return 0; +} + static int testVirPCIVPDParseFullVPDSkipInvalidKeywords(const void *opaque G_GNUC_UNUSED) { @@ -717,6 +818,33 @@ testVirPCIVPDParseFullVPDInvalid(const void *opaque G_GNUC_UNUSED) 'R', 'V', 0x02, 0x8A, 0x00, \ PCI_VPD_RESOURCE_END_VAL +/* The SN field has a length field that goes past the resource boundaries. */ +# define VPD_INVALID_SN_FIELD_LENGTH \ + VPD_STRING_RESOURCE_EXAMPLE_HEADER, \ + 't', 'e', 's', 't', 'n', 'a', 'm', 'e', \ + PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_READ_ONLY_LARGE_RESOURCE_FLAG, 0x0A, 0x00, \ + 'S', 'N', 0x42, 0x04, 0x02, \ + 'R', 'V', 0x02, 0xE8, 0x00, \ + PCI_VPD_RESOURCE_END_VAL + +/* The RV field is not the last one in VPD-R while the checksum is valid. */ +# define VPD_INVALID_RV_NOT_LAST \ + VPD_STRING_RESOURCE_EXAMPLE_HEADER, \ + 't', 'e', 's', 't', 'n', 'a', 'm', 'e', \ + PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_READ_ONLY_LARGE_RESOURCE_FLAG, 0x0A, 0x00, \ + 'R', 'V', 0x02, 0xD1, 0x00, \ + 'S', 'N', 0x02, 0x04, 0x02, \ + PCI_VPD_RESOURCE_END_VAL + +# define VPD_INVALID_RW_NOT_LAST \ + VPD_STRING_RESOURCE_EXAMPLE_HEADER, VPD_STRING_RESOURCE_EXAMPLE_DATA, \ + VPD_R_FIELDS_EXAMPLE_HEADER, VPD_R_FIELDS_EXAMPLE_DATA, \ + PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_READ_WRITE_LARGE_RESOURCE_FLAG, 0x08, 0x00, \ + 'R', 'W', 0x00, \ + 'V', 'Z', 0x02, '4', '2', \ + PCI_VPD_RESOURCE_END_VAL + + # define TEST_INVALID_VPD(invalidVPD) \ do { \ g_autoptr(virPCIVPDResource) res = NULL; \ @@ -741,6 +869,9 @@ testVirPCIVPDParseFullVPDInvalid(const void *opaque G_GNUC_UNUSED) TEST_INVALID_VPD(VPD_R_UNEXPECTED_RW_IN_VPD_R_KEY); TEST_INVALID_VPD(VPD_R_INVALID_FIELD_VALUE); TEST_INVALID_VPD(VPD_INVALID_STRING_RESOURCE_VALUE); + TEST_INVALID_VPD(VPD_INVALID_SN_FIELD_LENGTH); + TEST_INVALID_VPD(VPD_INVALID_RV_NOT_LAST); + TEST_INVALID_VPD(VPD_INVALID_RW_NOT_LAST); return 0; } @@ -767,6 +898,12 @@ mymain(void) ret = -1; if (virTestRun("Parsing VPD string resources ", testVirPCIVPDParseVPDStringResource, NULL) < 0) ret = -1; + if (virTestRun("Parsing a VPD resource with a zero-length RW ", + testVirPCIVPDParseZeroLengthRW, NULL) < 0) + ret = -1; + if (virTestRun("Parsing a VPD resource without an RW ", + testVirPCIVPDParseNoRW, NULL) < 0) + ret = -1; if (virTestRun("Parsing a VPD resource with an invalid keyword ", testVirPCIVPDParseFullVPDSkipInvalidKeywords, NULL) < 0) ret = -1;