While invalid values need to be ignored when presenting VPD data to the user, it would be good to attempt to parse a valid portion of the VPD instead of marking it invalid as a whole. Based on a mailing list discussion, the set of accepted characters is extended to the set of printable ASCII characters. https://listman.redhat.com/archives/libvir-list/2021-October/msg01043.html The particular example encountered on real hardware was multi-faceted: * "N/A" strings present in read-only fields. This would not be a useful valid value for a field (especially if a unique serial number is expected), however, it was decided to delegate handling of those kinds of values to higher-level software; * "4W/1W PCIeG2x4" - looks like some vendors use even more printable characters in the ASCII range than we currently allow. Since the PCI/PCIe VPD specs mention alphanumeric characters without specifying the full character set, it looks like this is ambiguous for vendors and they tend to use printable ASCII characters; * 0xFF bytes present in VPD-W field values. Those bytes do not map to printable ASCII code points and were probably used by the vendor as placeholders. Ignoring the whole VPD because of that would be too strict. Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@xxxxxxxxxxxxx> --- src/util/virpcivpd.c | 22 +++++--- tests/virpcivpdtest.c | 125 ++++++++++++++++++++++++++++++++++++++---- 2 files changed, 127 insertions(+), 20 deletions(-) diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index 4c96bc1a06..d8f2a43cde 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -161,8 +161,6 @@ virPCIVPDResourceGetFieldValueFormat(const char *keyword) return format; } -#define ACCEPTED_CHARS "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789 -_,.:;=" - /** * virPCIVPDResourceIsValidTextValue: * @value: A NULL-terminated string to assess. @@ -175,6 +173,7 @@ virPCIVPDResourceGetFieldValueFormat(const char *keyword) bool virPCIVPDResourceIsValidTextValue(const char *value) { + size_t i = 0; /* * The PCI(e) specs mention alphanumeric characters when talking about text fields * and the string resource but also include spaces and dashes in the provided example. @@ -191,10 +190,12 @@ virPCIVPDResourceIsValidTextValue(const char *value) if (STREQ(value, "")) return true; - if (strspn(value, ACCEPTED_CHARS) != strlen(value)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("The provided value contains invalid characters: %s"), value); - return false; + while (i < strlen(value)) { + if (!g_ascii_isprint(value[i])) { + VIR_DEBUG("The provided value contains non-ASCII printable characters: %s", value); + return false; + } + ++i; } return true; } @@ -544,9 +545,12 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re */ fieldValue = g_strstrip(g_strndup((char *)buf, fieldDataLen)); if (!virPCIVPDResourceIsValidTextValue(fieldValue)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Field value contains invalid characters")); - return false; + /* Skip fields with invalid values - this is safe assuming field length is + * correctly specified. */ + VIR_DEBUG("A value for field %s contains invalid characters", fieldKeyword); + g_free(g_steal_pointer(&fieldKeyword)); + g_free(g_steal_pointer(&fieldValue)); + continue; } } else if (fieldFormat == VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD) { if (*csum) { diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index a99bde2b92..b7bc86d922 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -322,8 +322,10 @@ testPCIVPDIsValidTextValue(const void *data G_GNUC_UNUSED) {"under_score_example", true}, {"", true}, {";", true}, - {"\\42", false}, - {"/42", false}, + {"\\42", true}, + {"N/A", true}, + /* The first and last code points are outside ASCII (multi-byte in UTF-8). */ + {"гbl🐧", false}, }; for (i = 0; i < sizeof(textValueCases) / sizeof(textValueCases[0]); ++i) { if (virPCIVPDResourceIsValidTextValue(textValueCases[i].keyword) != @@ -741,6 +743,113 @@ testVirPCIVPDParseFullVPDSkipInvalidKeywords(const void *opaque G_GNUC_UNUSED) return 0; } +static int +testVirPCIVPDParseFullVPDSkipInvalidValues(const void *opaque G_GNUC_UNUSED) +{ + int fd = -1; + size_t dataLen = 0; + size_t i = 0; + virPCIVPDResourceCustom *custom = NULL; + + g_autoptr(virPCIVPDResource) res = NULL; + + /* This example is based on real-world hardware which was programmed by the vendor with + * invalid field values in both the RO section and RW section. The RO section contains + * fields that are not valid per the spec but accepted by Libvirt as printable ASCII + * characters. The RW field has a 0 length which means there is no more space in the + * RW section. */ + const uint8_t fullVPDExample[] = { + 0x82, 0x23, 0x00, 0x48, 0x50, 0x20, 0x45, 0x74, 0x68, 0x65, 0x72, 0x6e, 0x65, 0x74, + 0x20, 0x31, 0x47, 0x62, 0x20, 0x32, 0x2d, 0x70, 0x6f, 0x72, 0x74, 0x20, 0x33, 0x36, + 0x31, 0x69, 0x20, 0x41, 0x64, 0x61, 0x70, 0x74, 0x65, 0x72, 0x90, 0x42, 0x00, 0x50, + 0x4e, 0x03, 0x4e, 0x2f, 0x41, 0x45, 0x43, 0x03, 0x4e, 0x2f, 0x41, 0x53, 0x4e, 0x03, + 0x4e, 0x2f, 0x41, 0x56, 0x30, 0x29, 0x34, 0x57, 0x2f, 0x31, 0x57, 0x20, 0x50, 0x43, + 0x49, 0x65, 0x47, 0x32, 0x78, 0x34, 0x20, 0x32, 0x70, 0x20, 0x31, 0x47, 0x62, 0x45, + 0x20, 0x52, 0x4a, 0x34, 0x35, 0x20, 0x49, 0x6e, 0x74, 0x65, 0x6c, 0x20, 0x69, 0x33, + 0x35, 0x30, 0x20, 0x20, 0x20, 0x52, 0x56, 0x01, 0x63, 0x91, 0x47, 0x00, 0x56, 0x31, + 0x06, 0x35, 0x2e, 0x37, 0x2e, 0x30, 0x36, 0x56, 0x33, 0x06, 0x32, 0x2e, 0x38, 0x2e, + 0x32, 0x30, 0x56, 0x36, 0x06, 0x31, 0x2e, 0x35, 0x2e, 0x33, 0x35, 0x59, 0x41, 0x03, + 0x4e, 0x2f, 0x41, 0x59, 0x42, 0x10, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x59, 0x43, 0x0D, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 'R', 'W', 0x00, 0x78, + }; + + 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; + } + /* Some values in the read-write section are invalid but parsing should succeed + * considering the parser is implemented to be graceful about invalid keywords and + * values. */ + if (!res->ro) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "The RO section consisting of only invalid fields got parsed successfully"); + return -1; + } + if (!res->rw) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "Could not successfully parse an RW section with some invalid fields"); + return -1; + } + + if (!STREQ_NULLABLE(res->ro->change_level, "N/A")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "Could not parse a change level field with acceptable contents"); + return -1; + } + if (!STREQ_NULLABLE(res->ro->part_number, "N/A")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "Could not parse a part number field with acceptable contents"); + return -1; + } + if (!STREQ_NULLABLE(res->ro->serial_number, "N/A")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "Could not parse a serial number with acceptable contents"); + return -1; + } + if (!STREQ_NULLABLE(res->rw->asset_tag, "N/A")) { + /* The asset tag has an invalid value in this case so it should be NULL. */ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "Could not parse an asset tag with acceptable contents"); + return -1; + } + if (res->rw->vendor_specific->len != 3) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "The number of parsed vendor fields is not equal to the expected number."); + return -1; + } + if (res->rw->system_specific->len > 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "Successfully parsed some systems-specific fields while none are valid"); + return -1; + } + for (i = 0; i < res->rw->vendor_specific->len; ++i) { + custom = ((virPCIVPDResourceCustom*)g_ptr_array_index(res->rw->vendor_specific, i)); + if (custom->idx == '1') { + if (STRNEQ(custom->value, "5.7.06")) { + return -1; + } + } else if (custom->idx == '3') { + if (STRNEQ(custom->value, "2.8.20")) { + return -1; + } + } else if (custom->idx == '6') { + if (STRNEQ(custom->value, "1.5.35")) { + return -1; + } + } + } + + return 0; +} + + static int testVirPCIVPDParseFullVPDInvalid(const void *opaque G_GNUC_UNUSED) { @@ -802,14 +911,6 @@ testVirPCIVPDParseFullVPDInvalid(const void *opaque G_GNUC_UNUSED) 'R', 'V', 0x02, 0x81, 0x00, \ PCI_VPD_RESOURCE_END_VAL -# define VPD_R_INVALID_FIELD_VALUE \ - VPD_STRING_RESOURCE_EXAMPLE_HEADER, \ - VPD_STRING_RESOURCE_EXAMPLE_DATA, \ - PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_READ_ONLY_LARGE_RESOURCE_FLAG, 0x0A, 0x00, \ - 'S', 'N', 0x02, 0x04, 0x02, \ - 'R', 'V', 0x02, 0x28, 0x00, \ - PCI_VPD_RESOURCE_END_VAL - # define VPD_INVALID_STRING_RESOURCE_VALUE \ VPD_STRING_RESOURCE_EXAMPLE_HEADER, \ 't', 0x03, 's', 't', 'n', 'a', 'm', 'e', \ @@ -867,7 +968,6 @@ testVirPCIVPDParseFullVPDInvalid(const void *opaque G_GNUC_UNUSED) TEST_INVALID_VPD(VPD_R_INVALID_RV); TEST_INVALID_VPD(VPD_R_INVALID_RV_ZERO_LENGTH); 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); @@ -904,6 +1004,9 @@ mymain(void) if (virTestRun("Parsing a VPD resource without an RW ", testVirPCIVPDParseNoRW, NULL) < 0) ret = -1; + if (virTestRun("Parsing a VPD resource with an invalid values ", + testVirPCIVPDParseFullVPDSkipInvalidValues, NULL) < 0) + ret = -1; if (virTestRun("Parsing a VPD resource with an invalid keyword ", testVirPCIVPDParseFullVPDSkipInvalidKeywords, NULL) < 0) ret = -1; -- 2.32.0
