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
