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@xxxxxxxxxxxxx>
> ---
> 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 :|
