Re: [libvirt PATCH 1/4] 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; * 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&gt; --- 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;