1:57 p.m.
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