[libvirt PATCH 3/4] PCI VPD: Skip fields with invalid values

[Daniel P. Berrangé]

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