lib: add PL011 to _CID whitelist
diff mbox series

Message ID 20181107041305.26905-1-alex.hung@canonical.com
State Rejected
Headers show
Series
  • lib: add PL011 to _CID whitelist
Related show

Commit Message

Alex Hung Nov. 7, 2018, 4:13 a.m. UTC
When attending UEFI Plugfest, a vendor suggested _CID PL011 is used in
the field, and this ID is added to a whitelist to avoid false positive.

Signed-off-by: Alex Hung <alex.hung@canonical.com>
---
 src/lib/src/fwts_acpi_object_eval.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Colin King Nov. 7, 2018, 7:52 a.m. UTC | #1
On 07/11/2018 04:13, Alex Hung wrote:
> When attending UEFI Plugfest, a vendor suggested _CID PL011 is used in
> the field, and this ID is added to a whitelist to avoid false positive.
> 
> Signed-off-by: Alex Hung <alex.hung@canonical.com>
> ---
>  src/lib/src/fwts_acpi_object_eval.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/src/lib/src/fwts_acpi_object_eval.c b/src/lib/src/fwts_acpi_object_eval.c
> index 0bc0f134..95f7df76 100644
> --- a/src/lib/src/fwts_acpi_object_eval.c
> +++ b/src/lib/src/fwts_acpi_object_eval.c
> @@ -968,6 +968,10 @@ bool fwts_method_valid_HID_string(char *str)
>  		return true;
>  	}
>  
> +	/* PL011 is used out there, so whitelist it */
> +	if (strlen(str) == 5 && !strncmp(str, "PL011", 5))
> +		return true;
> +
>  	return false;
>  }
>  
> 
Acked-by: Colin Ian King <colin.king@canonical.com>
ivanhu Nov. 7, 2018, 8:51 a.m. UTC | #2
I was in UEFI plugfest two, at first I think we can whitelist this ID, but

ACPI specification do define the type of the IDs,

A valid PNP ID must be of the form "AAA####" where A is an uppercase
letter and # is a hex
digit. A valid ACPI ID must be of the form "NNNN####" where N is an
uppercase letter or a
digit ('0'-'9') and # is a hex digit.

"PL011" indeed not follow the ACPI specification.

They know it is not following the ACPI specification, they cannot change
it because the legacy machine use it.

I think it maybe good idea let FWTS to spot all failures which not
following the ACPI specification, even if it is in used.

Any thoughts?


Cheers,

Ivan

On 11/7/18 12:13 PM, Alex Hung wrote:
> When attending UEFI Plugfest, a vendor suggested _CID PL011 is used in
> the field, and this ID is added to a whitelist to avoid false positive.
>
> Signed-off-by: Alex Hung <alex.hung@canonical.com>
> ---
>  src/lib/src/fwts_acpi_object_eval.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/src/lib/src/fwts_acpi_object_eval.c b/src/lib/src/fwts_acpi_object_eval.c
> index 0bc0f134..95f7df76 100644
> --- a/src/lib/src/fwts_acpi_object_eval.c
> +++ b/src/lib/src/fwts_acpi_object_eval.c
> @@ -968,6 +968,10 @@ bool fwts_method_valid_HID_string(char *str)
>  		return true;
>  	}
>  
> +	/* PL011 is used out there, so whitelist it */
> +	if (strlen(str) == 5 && !strncmp(str, "PL011", 5))
> +		return true;
> +
>  	return false;
>  }
>
Alex Hung Nov. 7, 2018, 9:13 a.m. UTC | #3
On Wed, Nov 7, 2018 at 4:52 PM ivanhu <ivan.hu@canonical.com> wrote:
>
> I was in UEFI plugfest two, at first I think we can whitelist this ID, but
>
> ACPI specification do define the type of the IDs,
>
> A valid PNP ID must be of the form "AAA####" where A is an uppercase
> letter and # is a hex
> digit. A valid ACPI ID must be of the form "NNNN####" where N is an
> uppercase letter or a
> digit ('0'-'9') and # is a hex digit.
>
> "PL011" indeed not follow the ACPI specification.
>
> They know it is not following the ACPI specification, they cannot change
> it because the legacy machine use it.
>
> I think it maybe good idea let FWTS to spot all failures which not
> following the ACPI specification, even if it is in used.
>
> Any thoughts?

It is unlikely to undo the used CID. The failures will always be
there, even if the CID is supported by firmware and OS. This failure
will make little sense as it may never get fixed.

>
>
> Cheers,
>
> Ivan
>
> On 11/7/18 12:13 PM, Alex Hung wrote:
> > When attending UEFI Plugfest, a vendor suggested _CID PL011 is used in
> > the field, and this ID is added to a whitelist to avoid false positive.
> >
> > Signed-off-by: Alex Hung <alex.hung@canonical.com>
> > ---
> >  src/lib/src/fwts_acpi_object_eval.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/src/lib/src/fwts_acpi_object_eval.c b/src/lib/src/fwts_acpi_object_eval.c
> > index 0bc0f134..95f7df76 100644
> > --- a/src/lib/src/fwts_acpi_object_eval.c
> > +++ b/src/lib/src/fwts_acpi_object_eval.c
> > @@ -968,6 +968,10 @@ bool fwts_method_valid_HID_string(char *str)
> >               return true;
> >       }
> >
> > +     /* PL011 is used out there, so whitelist it */
> > +     if (strlen(str) == 5 && !strncmp(str, "PL011", 5))
> > +             return true;
> > +
> >       return false;
> >  }
> >
>
> --
> fwts-devel mailing list
> fwts-devel@lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/fwts-devel
Al Stone Nov. 8, 2018, 12:18 a.m. UTC | #4
On 11/7/18 1:51 AM, ivanhu wrote:
> I was in UEFI plugfest two, at first I think we can whitelist this ID, but
> 
> ACPI specification do define the type of the IDs,
> 
> A valid PNP ID must be of the form "AAA####" where A is an uppercase
> letter and # is a hex
> digit. A valid ACPI ID must be of the form "NNNN####" where N is an
> uppercase letter or a
> digit ('0'-'9') and # is a hex digit.
> 
> "PL011" indeed not follow the ACPI specification.
> 
> They know it is not following the ACPI specification, they cannot change
> it because the legacy machine use it.
> 
> I think it maybe good idea let FWTS to spot all failures which not
> following the ACPI specification, even if it is in used.
> 
> Any thoughts?

Section 6.1.2 of the spec is pretty clear on what is allowed for _CID; 'PL011'
is not allowed.  It is not in the PCI or ACPI ID registries, either.

I have seen this used in some very, very old firmware, and the vendor that made
the firmware was forced to correct it as soon as I saw it.  So, that firmware
has since been corrected, and there is no excuse for it to still be in use, even
on a "legacy system."

The value should be 'ARMH0011', so this is broken firmware, and if the kernel is
accepting 'PL011', that's wrong, too.

Vendors should not be using this at all.  The test should fail.

> Cheers,
> 
> Ivan
> 
> On 11/7/18 12:13 PM, Alex Hung wrote:
>> When attending UEFI Plugfest, a vendor suggested _CID PL011 is used in
>> the field, and this ID is added to a whitelist to avoid false positive.
>>
>> Signed-off-by: Alex Hung <alex.hung@canonical.com>
>> ---
>>  src/lib/src/fwts_acpi_object_eval.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/src/lib/src/fwts_acpi_object_eval.c b/src/lib/src/fwts_acpi_object_eval.c
>> index 0bc0f134..95f7df76 100644
>> --- a/src/lib/src/fwts_acpi_object_eval.c
>> +++ b/src/lib/src/fwts_acpi_object_eval.c
>> @@ -968,6 +968,10 @@ bool fwts_method_valid_HID_string(char *str)
>>  		return true;
>>  	}
>>  
>> +	/* PL011 is used out there, so whitelist it */
>> +	if (strlen(str) == 5 && !strncmp(str, "PL011", 5))
>> +		return true;
>> +
>>  	return false;
>>  }
>>  
> 
>
Alex Hung Nov. 8, 2018, 1:05 a.m. UTC | #5
On Thu, Nov 8, 2018 at 8:18 AM Al Stone <al.stone@linaro.org> wrote:
>
> On 11/7/18 1:51 AM, ivanhu wrote:
> > I was in UEFI plugfest two, at first I think we can whitelist this ID, but
> >
> > ACPI specification do define the type of the IDs,
> >
> > A valid PNP ID must be of the form "AAA####" where A is an uppercase
> > letter and # is a hex
> > digit. A valid ACPI ID must be of the form "NNNN####" where N is an
> > uppercase letter or a
> > digit ('0'-'9') and # is a hex digit.
> >
> > "PL011" indeed not follow the ACPI specification.
> >
> > They know it is not following the ACPI specification, they cannot change
> > it because the legacy machine use it.
> >
> > I think it maybe good idea let FWTS to spot all failures which not
> > following the ACPI specification, even if it is in used.
> >
> > Any thoughts?
>
> Section 6.1.2 of the spec is pretty clear on what is allowed for _CID; 'PL011'
> is not allowed.  It is not in the PCI or ACPI ID registries, either.
>
> I have seen this used in some very, very old firmware, and the vendor that made
> the firmware was forced to correct it as soon as I saw it.  So, that firmware
> has since been corrected, and there is no excuse for it to still be in use, even
> on a "legacy system."
>
> The value should be 'ARMH0011', so this is broken firmware, and if the kernel is
> accepting 'PL011', that's wrong, too.
>
> Vendors should not be using this at all.  The test should fail.

Thanks Al for the clarification. Now I know what to ask vendors to change to.

>
> > Cheers,
> >
> > Ivan
> >
> > On 11/7/18 12:13 PM, Alex Hung wrote:
> >> When attending UEFI Plugfest, a vendor suggested _CID PL011 is used in
> >> the field, and this ID is added to a whitelist to avoid false positive.
> >>
> >> Signed-off-by: Alex Hung <alex.hung@canonical.com>
> >> ---
> >>  src/lib/src/fwts_acpi_object_eval.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/src/lib/src/fwts_acpi_object_eval.c b/src/lib/src/fwts_acpi_object_eval.c
> >> index 0bc0f134..95f7df76 100644
> >> --- a/src/lib/src/fwts_acpi_object_eval.c
> >> +++ b/src/lib/src/fwts_acpi_object_eval.c
> >> @@ -968,6 +968,10 @@ bool fwts_method_valid_HID_string(char *str)
> >>              return true;
> >>      }
> >>
> >> +    /* PL011 is used out there, so whitelist it */
> >> +    if (strlen(str) == 5 && !strncmp(str, "PL011", 5))
> >> +            return true;
> >> +
> >>      return false;
> >>  }
> >>
> >
> >
>
>
> --
> ciao,
> al
> -----------------------------------
> Al Stone
> Software Engineer
> Linaro Enterprise Group
> al.stone@linaro.org
> -----------------------------------
>
> --
> fwts-devel mailing list
> fwts-devel@lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/fwts-devel

Patch
diff mbox series

diff --git a/src/lib/src/fwts_acpi_object_eval.c b/src/lib/src/fwts_acpi_object_eval.c
index 0bc0f134..95f7df76 100644
--- a/src/lib/src/fwts_acpi_object_eval.c
+++ b/src/lib/src/fwts_acpi_object_eval.c
@@ -968,6 +968,10 @@  bool fwts_method_valid_HID_string(char *str)
 		return true;
 	}
 
+	/* PL011 is used out there, so whitelist it */
+	if (strlen(str) == 5 && !strncmp(str, "PL011", 5))
+		return true;
+
 	return false;
 }