1:57 p.m.
This patch set improves edge case testing:
* The parser is now more strict about checking boundary conditions when
parsing fields: an invalid field length is a possibility which is
now being accounted for;
* The parser will now make sure that RV and RW fields are the last
in their section by making sure that no more data is left to read
after those;
* The RW field in the read-write section is not considered a VPD format
violation even though it is a violation of the spec since it does not
prevent Libvirt from parsing valid data for presenting it to a user.
This is a policy decision made by Libvirt in favor of usability with
hardware that does not strictly follow the PCI/PCIe VPD spec.
Invalid field values are now skipped instead of halting further parsing
completely.
Some vendors use 0xFF as placeholders in VPD-W since those values do not
correspond to printable ASCII characters, they will be discarded,
however, parsing will continue beyond that point.
Also, it turns out that some vendors use printable ASCII characters not
present in the alphanumeric range. Following a mailing list discussion
Libvirt will accept printable ASCII characters to avoid cases where
useful data is discarded.
https://listman.redhat.com/archives/libvir-list/2021-October/msg01043.html
Higher-level software needs to account for this character set and act
accordingly.
For example, the outcome of this is that one may get "N/A" as a value
for a serial number that is supposed to be unique, however, there is
no way for Libvirt to validate serial number uniqueness anyway even if
it was a different character sequence.
https://gitlab.com/dmitriis/libvirt/-/pipelines/398517951
(x86_64 only, have not set up arch-specific runners yet and over the
limit of what gitlab provides)
Dmitrii Shcherbakov (3):
PCI VPD: handle additional edge cases
PCI VPD: Skip fields with invalid values
PCI VPD: Fix a wrong return code in a test case
src/util/virpcivpd.c | 63 +++++++---
tests/virpcivpdtest.c | 263 ++++++++++++++++++++++++++++++++++++++++--
2 files changed, 296 insertions(+), 30 deletions(-)
--
2.32.0
1:57 p.m.
New subject: [libvirt PATCH v2 1/3] 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.
Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov(a)canonical.com>
---
src/util/virpcivpd.c | 41 ++++++++++---
tests/virpcivpdtest.c | 137 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 169 insertions(+), 9 deletions(-)
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;
--
2.32.0
1:57 p.m.
New subject: [libvirt PATCH v2 2/3] 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.
Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov(a)canonical.com>
---
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
1:57 p.m.
New subject: [libvirt PATCH v2 3/3] PCI VPD: Fix a wrong return code in a test case
The test case should return -1, not 0 in case a valid resource could
not be parsed successfully but the ret value is initialized to 0.
Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov(a)canonical.com>
---
tests/virpcivpdtest.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c
index b7bc86d922..a9405f9427 100644
--- a/tests/virpcivpdtest.c
+++ b/tests/virpcivpdtest.c
@@ -537,7 +537,6 @@ testVirPCIVPDParseFullVPD(const void *opaque G_GNUC_UNUSED)
{
int fd = -1;
size_t dataLen = 0;
- int ret = 0;
g_autoptr(virPCIVPDResource) res = NULL;
/* Note: Custom fields are supposed to be freed by the resource cleanup code. */
@@ -558,7 +557,7 @@ testVirPCIVPDParseFullVPD(const void *opaque G_GNUC_UNUSED)
if (!res) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
"The resource pointer is NULL after parsing which is
unexpected");
- return ret;
+ return -1;
}
if (!res->ro) {
@@ -596,7 +595,7 @@ testVirPCIVPDParseFullVPD(const void *opaque G_GNUC_UNUSED)
return -1;
custom = NULL;
- return ret;
+ return 0;
}
static int
--
2.32.0
7:34 p.m.
On 10/29/21 15:57, Dmitrii Shcherbakov wrote:
This patch set improves edge case testing:
* The parser is now more strict about checking boundary conditions when
parsing fields: an invalid field length is a possibility which is
now being accounted for;
* The parser will now make sure that RV and RW fields are the last
in their section by making sure that no more data is left to read
after those;
* The RW field in the read-write section is not considered a VPD format
violation even though it is a violation of the spec since it does not
prevent Libvirt from parsing valid data for presenting it to a user.
This is a policy decision made by Libvirt in favor of usability with
hardware that does not strictly follow the PCI/PCIe VPD spec.
Libvirtd in a Power9 host is throwing these warnings on every startup:
Running 'src/libvirtd'...
2021-10-31 23:56:40.637+0000: 140468: info : libvirt version: 7.9.0
2021-10-31 23:56:40.637+0000: 140468: info : hostname: ltc-boston118
2021-10-31 23:56:40.804+0000: 140520: error : virPCIVPDParseVPDLargeResourceFields:588 :
internal error: VPD-W does not contain the mandatory RW field
2021-10-31 23:56:40.808+0000: 140520: error : virPCIVPDParseVPDLargeResourceFields:588 :
internal error: VPD-W does not contain the mandatory RW field
2021-10-31 23:56:40.813+0000: 140520: error : virPCIVPDParseVPDLargeResourceFields:588 :
internal error: VPD-W does not contain the mandatory RW field
2021-10-31 23:56:40.817+0000: 140520: error : virPCIVPDParseVPDLargeResourceFields:588 :
internal error: VPD-W does not contain the mandatory RW field
With this series the warnings are now gone.
All patches:
Tested-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
Invalid field values are now skipped instead of halting further parsing
completely.
Some vendors use 0xFF as placeholders in VPD-W since those values do not
correspond to printable ASCII characters, they will be discarded,
however, parsing will continue beyond that point.
Also, it turns out that some vendors use printable ASCII characters not
present in the alphanumeric range. Following a mailing list discussion
Libvirt will accept printable ASCII characters to avoid cases where
useful data is discarded.
https://listman.redhat.com/archives/libvir-list/2021-October/msg01043.html
Higher-level software needs to account for this character set and act
accordingly.
For example, the outcome of this is that one may get "N/A" as a value
for a serial number that is supposed to be unique, however, there is
no way for Libvirt to validate serial number uniqueness anyway even if
it was a different character sequence.
https://gitlab.com/dmitriis/libvirt/-/pipelines/398517951
(x86_64 only, have not set up arch-specific runners yet and over the
limit of what gitlab provides)
Dmitrii Shcherbakov (3):
PCI VPD: handle additional edge cases
PCI VPD: Skip fields with invalid values
PCI VPD: Fix a wrong return code in a test case
src/util/virpcivpd.c | 63 +++++++---
tests/virpcivpdtest.c | 263 ++++++++++++++++++++++++++++++++++++++++--
2 files changed, 296 insertions(+), 30 deletions(-)
1116
days inactive
1119
days old
4 comments
2 participants
participants (2)
-
Daniel Henrique Barboza -
Dmitrii Shcherbakov