[libvirt PATCH 1/4] PCI VPD: handle additional edge cases

[Daniel P. Berrangé]

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 at 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 :|