[libvirt PATCH 0/4] PCI VPD: Handle More Edge Cases
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; * RW field presence was already checked for but there was no test case for it. This was prompted by a real-world hardware example where this field was not present. Invalid field values are now skipped instead of halting further parsing completely. It turns out that some vendors use values like "N/A" if fields were not populated in VPD-R and 0xFF as placeholders in VPD-W. While this data is not valid, other data might be and it should be possible to retrieve it. Dmitrii Shcherbakov (4): PCI VPD: handle additional edge cases PCI VPD: Add a test case for the absence of RW PCI VPD: Skip fields with invalid values PCI VPD: Fix a wrong return code in a test case src/util/virpcivpd.c | 37 ++++++-- tests/virpcivpdtest.c | 207 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 225 insertions(+), 19 deletions(-) -- 2.32.0
* 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; * 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@canonical.com> --- src/util/virpcivpd.c | 28 +++++++++++---- tests/virpcivpdtest.c | 84 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 6 deletions(-) diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index 8856bca459..cd49031fa4 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,13 +591,17 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re g_free(g_steal_pointer(&fieldKeyword)); g_free(g_steal_pointer(&fieldValue)); } - if (readOnly && !hasChecksum) { + + /* 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)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("VPD-R does not contain the mandatory RV field")); + _("VPD-R does not contain the mandatory RV field as the last field")); return false; - } else if (!readOnly && !hasRW) { + } else if (!readOnly && !(hasRW && endReached)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("VPD-W does not contain the mandatory RW field")); + _("VPD-W does not contain the mandatory RW field as the last field")); return false; } diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index 2cc9069132..65607c1247 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -597,6 +597,58 @@ 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 testVirPCIVPDParseFullVPDSkipInvalidKeywords(const void *opaque G_GNUC_UNUSED) { @@ -717,6 +769,32 @@ 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 +819,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 +848,9 @@ 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 with an invalid keyword ", testVirPCIVPDParseFullVPDSkipInvalidKeywords, NULL) < 0) ret = -1; -- 2.32.0
On Fri, Oct 22, 2021 at 03:45:42PM +0300, Dmitrii Shcherbakov wrote:
* 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; * 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@canonical.com> --- src/util/virpcivpd.c | 28 +++++++++++---- tests/virpcivpdtest.c | 84 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 6 deletions(-)
diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index 8856bca459..cd49031fa4 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,13 +591,17 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re g_free(g_steal_pointer(&fieldKeyword)); g_free(g_steal_pointer(&fieldValue)); } - if (readOnly && !hasChecksum) { + + /* 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)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("VPD-R does not contain the mandatory RV field")); + _("VPD-R does not contain the mandatory RV field as the last field")); return false; - } else if (!readOnly && !hasRW) { + } else if (!readOnly && !(hasRW && endReached)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("VPD-W does not contain the mandatory RW field")); + _("VPD-W does not contain the mandatory RW field as the last field")); return false;
Something is still not right with the logic here as this error report is triggering on my machines. If I comment out this check, then I can get data reported by libvirt Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Hi Daniel,
Something is still not right with the logic here as this error report is triggering on my machines. If I comment out this check, then I can get data reported by libvirt
The VPD example you shared previously has multiple violations of the VPD format: * Invalid field values ("N/A" in the read-only section, 0xFF in the read-write section); * The lack of the RW field at the end of the read-write section. Previously, the VPD was discarded due to invalid values. I thought honoring the RW requirement would still make sense and kept the RW presence check. I added it as a test case here (converted to hex): https://listman.redhat.com/archives/libvir-list/2021-October/msg00871.html The spec says the following (PCIe 4.0 "6.28.2.3 Read/Write Fields"): "RW | Remaining Read/Write Area | This descriptor is used to identify the unused portion of the read/write space. The product vendor initializes this parameter based on the size of the read/write space or the space remaining following the Vx VPD items. One or more of the Vx, Yx, and RW items are required." I can also confirm that by looking at the hexdump and looking for hex values for R (0x52) and W (0x57): hexdump -e '16/1 "0x%02x, " "\n"' vpd I could ease it up and allow it to be missing as we do not care much about it when reading so long as the resource length is correct. However, if we were to allow Libvirt to write to the read-write section in the future we would need to know how much free space is there in the first place.
On Tue, Oct 26, 2021 at 08:28:20PM +0300, Dmitrii Shcherbakov wrote:
Hi Daniel,
Something is still not right with the logic here as this error report is triggering on my machines. If I comment out this check, then I can get data reported by libvirt
The VPD example you shared previously has multiple violations of the VPD format:
* Invalid field values ("N/A" in the read-only section, 0xFF in the read-write section); * The lack of the RW field at the end of the read-write section.
Previously, the VPD was discarded due to invalid values.
I thought honoring the RW requirement would still make sense and kept the RW presence check.
I added it as a test case here (converted to hex): https://listman.redhat.com/archives/libvir-list/2021-October/msg00871.html
The spec says the following (PCIe 4.0 "6.28.2.3 Read/Write Fields"):
"RW | Remaining Read/Write Area | This descriptor is used to identify the unused portion of the read/write space. The product vendor initializes this parameter based on the size of the read/write space or the space remaining following the Vx VPD items. One or more of the Vx, Yx, and RW items are required."
I can also confirm that by looking at the hexdump and looking for hex values for R (0x52) and W (0x57):
hexdump -e '16/1 "0x%02x, " "\n"' vpd
I could ease it up and allow it to be missing as we do not care much about it when reading so long as the resource length is correct. However, if we were to allow Libvirt to write to the read-write section in the future we would need to know how much free space is there in the first place.
The issue I have is that 'lspci -vvv' will happily report the VPD data on my machine: Capabilities: [e0] Vital Product Data Product Name: HP Ethernet 1Gb 2-port 361i Adapter Read-only fields: [PN] Part number: N/A [EC] Engineering changes: N/A [SN] Serial number: N/A [V0] Vendor specific: 4W/1W PCIeG2x4 2p 1GbE RJ45 Intel i350 [RV] Reserved: checksum good, 0 byte(s) reserved Read/write fields: [V1] Vendor specific: 5.7.06 [V3] Vendor specific: 2.8.20 [V6] Vendor specific: 1.5.35 [YA] Asset tag: N/A [YB] System specific: <FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF> [YC] System specific: <FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF> End while libvirt refuses to report it. Even if the BIOS is not perfectly following the spec, it is clearly still possible to extract the data and display it to the user. So I don't think it is reasonable for libvirt to refuse todo this. I'd have sympathy if this machine was pre-production hardware or from a no-name vendor, but this machine is typical production quality hardware from a major vendor. The only think there I think it is reasonable for libvirt to reject the data is in the cae of the two fields that contain non-printable characters <FF>. Other than that I expect libvirt to report exactly the same data as lspci will report, and not try to secondguess whether the values provided by the vendor are semantically valid or not. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel,
The issue I have is that 'lspci -vvv' will happily report the VPD data on my machine:
...
while libvirt refuses to report it. Even if the BIOS is not perfectly following the spec, it is clearly still possible to extract the data and display it to the user. So I don't think it is reasonable for libvirt to refuse todo this. I'd have sympathy if this machine was pre-production hardware or from a no-name vendor, but this machine is typical production quality hardware from a major vendor.
The only think there I think it is reasonable for libvirt to reject the data is in the cae of the two fields that contain non-printable characters <FF>. Other than that I expect libvirt to report exactly the same data as lspci will report, and not try to secondguess whether the values provided by the vendor are semantically valid or not.
Makes sense to me: we can make a policy decision that better fits users and the reality of existing hardware. I also recognize that the VPD spec is quite old and it does not include a very good description of the supported character set (besides saying "alphanumeric"). I tried to follow the spec relatively strictly but I agree that a balance between that and providing useful information is needed. I'll resend the series and restrict the character set to the set of printable ASCII code points and ignore the lack of RW at the end of the read-write section. Best Regards, Dmitrii Shcherbakov LP/oftc: dmitriis
The code already handles a missing RW, however, there was no test case for that. It would be good to have it since a VPD like this was encountered on a real-world device. Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com> --- tests/virpcivpdtest.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index 65607c1247..00e34cc94a 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -795,6 +795,22 @@ testVirPCIVPDParseFullVPDInvalid(const void *opaque G_GNUC_UNUSED) 'V', 'Z', 0x02, '4', '2', \ PCI_VPD_RESOURCE_END_VAL +# define VPD_INVALID_NO_RW \ + 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, 0x10, 0xFF, 0xFF, 0xFF, \ + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x78 + + # define TEST_INVALID_VPD(invalidVPD) \ do { \ g_autoptr(virPCIVPDResource) res = NULL; \ @@ -822,6 +838,7 @@ testVirPCIVPDParseFullVPDInvalid(const void *opaque G_GNUC_UNUSED) TEST_INVALID_VPD(VPD_INVALID_SN_FIELD_LENGTH); TEST_INVALID_VPD(VPD_INVALID_RV_NOT_LAST); TEST_INVALID_VPD(VPD_INVALID_RW_NOT_LAST); + TEST_INVALID_VPD(VPD_INVALID_NO_RW); return 0; } -- 2.32.0
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@canonical.com> --- 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
On Fri, Oct 22, 2021 at 03:45:44PM +0300, Dmitrii Shcherbakov wrote:
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;
The higher level software needs to acccept that the BIOS vendor might have included garbage and have a plan to deal with this. Libvirt should apply a policy in this regards as it cann result in libvirt rejecting valid data. For example by rejecting "/" as a character, we fail to report valid data from mymachine. If I allow "/" I get: <capability type='vpd'> <name>HP Ethernet 1Gb 2-port 361i Adapter</name> <fields access='readonly'> <change_level>N/A</change_level> <part_number>N/A</part_number> <serial_number>N/A</serial_number> <vendor_field index='0'>4W/1W PCIeG2x4 2p 1GbE RJ45 Intel i350</vendor_field> </fields> <fields access='readwrite'> <asset_tag>N/A</asset_tag> <vendor_field index='1'>5.7.06</vendor_field> <vendor_field index='3'>2.8.20</vendor_field> <vendor_field index='6'>1.5.35</vendor_field> </fields> </capability> sure the "n/a" is unhelpful, but libvirt is right to include it. I think perhaps rather than allow-listing the characters, we need to just accept any field value that contains printable characters. eg instead of the strspn, I'd suggest we do as "isprint()" check on each character
* 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@canonical.com> --- 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)) {
This method itself calls virReportError, so we still get stuff reported at level ERROR in the logs. We need to remove the virReportError call in this method.
- 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);
We should include fieldValue in the log, even if it might have non-printable chars, as it'll be important debugging info.
+ 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) {
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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@canonical.com> --- tests/virpcivpdtest.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index b56f335fab..a4e9b2d0e6 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -535,7 +535,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. */ @@ -556,7 +555,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) { @@ -594,7 +593,7 @@ testVirPCIVPDParseFullVPD(const void *opaque G_GNUC_UNUSED) return -1; custom = NULL; - return ret; + return 0; } static int -- 2.32.0
participants (2)
-
Daniel P. Berrangé -
Dmitrii Shcherbakov