PCI VPD: handle additional edge cases

* RV and RW fields must be at the last position in their respective
  section (per the conditions in the spec). Therefore, the parser now
  stops iterating over fields as soon as it encounters one of those
  fields and checks whether the end of the resource has been reached;
* The lack of the RW field is not treated as a parsing error since we
  can still extract valid data even though this is a PCI/PCIe VPD spec
  violation;
* Individual fields must have a valid length - the parser needs to check
  for invalid length values that violate boundary conditions of the
  resource.
* A zero-length field may be the last one in the resource, however, the
  boundary check is currently too strict to allow that.

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 8856bca459595c9c90d9c085d3c505aa21bfc562..4c96bc1a069e68f09c539b301c28e6f7a964a36e 100644 (file)
--- 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 2cc906913209c2f0c36839cb004c71e8cd73c995..a99bde2b92825192d2749efc5ebb4c2b4932b14a 100644 (file)
--- 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;