PCI VPD: Skip fields with invalid values

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.

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com>

diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c
index 4c96bc1a069e68f09c539b301c28e6f7a964a36e..d8f2a43cde45929be233f7db138c2cd11466f375 100644 (file)
--- 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 a99bde2b92825192d2749efc5ebb4c2b4932b14a..b7bc86d92297d673f0cb62f43f9fbe1afa101f2f 100644 (file)
--- 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;