[libvirt PATCH 3/4] PCI VPD: Skip fields with invalid values

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

The particular example encountered on real hardware was twofold:

* "N/A" strings present in read-only fields. This would not be a useful
  valid value for a field, not to mention that if the serial number
  field with the "N/A" value was parsed, it would confuse higher-level
  software because this isn't a unique serial for a device;
* 0xFF bytes present in VPD-W field values. Those bytes are not valid
  values 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  |   9 ++--
 tests/virpcivpdtest.c | 105 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 102 insertions(+), 12 deletions(-)

diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c
index cd49031fa4..8c2b17c3a6 100644
--- a/src/util/virpcivpd.c
+++ b/src/util/virpcivpd.c
@@ -544,9 +544,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 00e34cc94a..b56f335fab 100644
--- a/tests/virpcivpdtest.c
+++ b/tests/virpcivpdtest.c
@@ -692,6 +692,99 @@ 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
+     * only fields with invalid values and the checksum field with a correct checksum. 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;
+    }
+    /* In this case none of the RO fields have valid contents, however, they have been checksummed
+     * correctly. Therefore, the RO portion of the resource is not initialized. The checksum
+     * includes bytes of the device name besides the VPD-R section. So this example is valid
+     * and, considering the parser is implemented to be graceful about invalid keywords and
+     * values, this situation is generally possible. */
+    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 (res->rw->asset_tag) {
+        /* The asset tag has an invalid value in this case so it should be NULL. */
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       "Successfully parsed an invalid asset tag.");
+        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)
 {
@@ -753,14 +846,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', \
@@ -833,7 +918,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);
@@ -868,6 +952,9 @@ mymain(void)
     if (virTestRun("Parsing a VPD resource with a zero-length RW ",
                    testVirPCIVPDParseZeroLengthRW, 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





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux