[PATCH v2] ACPI / PNP: check the string length of pnp device id in matching_id

[PATCH v2] ACPI / PNP: check the string length of pnp device id in matching_id

[Hui Wang]

Recently we met a touchscreen problem on some Thinkpad machines, the
touchscreen driver (i2c-hid) is not loaded and the touchscreen can't
work.

An i2c ACPI device with the name WACF2200 is defined in the BIOS, with
the current ACPI PNP matching rule, this device will be regarded as
a PNP device since there is WACFXXX in the acpi_pnp_device_ids[] and
this PNP device is attached to the acpi device as the 1st
physical_node, this will make the i2c bus match fail when i2c bus
calls acpi_companion_match() to match the acpi_id_table in the i2c-hid
driver.

An ACPI PNP device's id has fixed format and its string length equals
7, after adding this check in the matching_id, the touchscreen could
work.

Cc: stable@vger.kernel.org
Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
 drivers/acpi/acpi_pnp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/acpi/acpi_pnp.c b/drivers/acpi/acpi_pnp.c
index 4ed755a963aa..5ce711b9b070 100644
--- a/drivers/acpi/acpi_pnp.c
+++ b/drivers/acpi/acpi_pnp.c
@@ -319,6 +319,10 @@ static bool matching_id(const char *idstr, const char *list_id)
 {
 	int i;
 
+	/* a pnp device id has CCCdddd format (C character, d digit), strlen should be 7 */
+	if (strlen(idstr) != 7)
+		return false;
+
 	if (memcmp(idstr, list_id, 3))
 		return false;
 
-- 
2.25.1

Re: [PATCH v2] ACPI / PNP: check the string length of pnp device id in matching_id

[Greg KH]

On Thu, Dec 10, 2020 at 09:25:39AM +0800, Hui Wang wrote:
> Recently we met a touchscreen problem on some Thinkpad machines, the
> touchscreen driver (i2c-hid) is not loaded and the touchscreen can't
> work.
> 
> An i2c ACPI device with the name WACF2200 is defined in the BIOS, with
> the current ACPI PNP matching rule, this device will be regarded as
> a PNP device since there is WACFXXX in the acpi_pnp_device_ids[] and
> this PNP device is attached to the acpi device as the 1st
> physical_node, this will make the i2c bus match fail when i2c bus
> calls acpi_companion_match() to match the acpi_id_table in the i2c-hid
> driver.
> 
> An ACPI PNP device's id has fixed format and its string length equals
> 7, after adding this check in the matching_id, the touchscreen could
> work.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
>  drivers/acpi/acpi_pnp.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/acpi/acpi_pnp.c b/drivers/acpi/acpi_pnp.c
> index 4ed755a963aa..5ce711b9b070 100644
> --- a/drivers/acpi/acpi_pnp.c
> +++ b/drivers/acpi/acpi_pnp.c
> @@ -319,6 +319,10 @@ static bool matching_id(const char *idstr, const char *list_id)
>  {
>  	int i;
>  
> +	/* a pnp device id has CCCdddd format (C character, d digit), strlen should be 7 */
> +	if (strlen(idstr) != 7)
> +		return false;

Shouldn't you verify that the format is correct as well?

thanks,

greg k-h

Re: [PATCH v2] ACPI / PNP: check the string length of pnp device id in matching_id

[Rafael J. Wysocki]

On Thu, Dec 10, 2020 at 2:27 AM Hui Wang <hui.wang@canonical.com> wrote:
>
> Recently we met a touchscreen problem on some Thinkpad machines, the
> touchscreen driver (i2c-hid) is not loaded and the touchscreen can't
> work.
>
> An i2c ACPI device with the name WACF2200 is defined in the BIOS, with
> the current ACPI PNP matching rule, this device will be regarded as
> a PNP device since there is WACFXXX in the acpi_pnp_device_ids[] and
> this PNP device is attached to the acpi device as the 1st
> physical_node, this will make the i2c bus match fail when i2c bus
> calls acpi_companion_match() to match the acpi_id_table in the i2c-hid
> driver.
>
> An ACPI PNP device's id has fixed format and its string length equals
> 7, after adding this check in the matching_id, the touchscreen could
> work.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
>  drivers/acpi/acpi_pnp.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/acpi/acpi_pnp.c b/drivers/acpi/acpi_pnp.c
> index 4ed755a963aa..5ce711b9b070 100644
> --- a/drivers/acpi/acpi_pnp.c
> +++ b/drivers/acpi/acpi_pnp.c
> @@ -319,6 +319,10 @@ static bool matching_id(const char *idstr, const char *list_id)
>  {
>         int i;
>
> +       /* a pnp device id has CCCdddd format (C character, d digit), strlen should be 7 */
> +       if (strlen(idstr) != 7)
> +               return false;

What I meant was comparing the length of the arg strings and return
false if they don't match.  Wouldn't that work?

> +
>         if (memcmp(idstr, list_id, 3))
>                 return false;
>
> --
> 2.25.1
>

Re: [PATCH v2] ACPI / PNP: check the string length of pnp device id in matching_id

[Hui Wang]

On 12/10/20 5:04 PM, Greg KH wrote:
> On Thu, Dec 10, 2020 at 09:25:39AM +0800, Hui Wang wrote:
>> Recently we met a touchscreen problem on some Thinkpad machines, the
>> touchscreen driver (i2c-hid) is not loaded and the touchscreen can't
>> work.
>>
>> An i2c ACPI device with the name WACF2200 is defined in the BIOS, with
>> the current ACPI PNP matching rule, this device will be regarded as
>> a PNP device since there is WACFXXX in the acpi_pnp_device_ids[] and
>> this PNP device is attached to the acpi device as the 1st
>> physical_node, this will make the i2c bus match fail when i2c bus
>> calls acpi_companion_match() to match the acpi_id_table in the i2c-hid
>> driver.
>>
>> An ACPI PNP device's id has fixed format and its string length equals
>> 7, after adding this check in the matching_id, the touchscreen could
>> work.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>> ---
>>   drivers/acpi/acpi_pnp.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/acpi/acpi_pnp.c b/drivers/acpi/acpi_pnp.c
>> index 4ed755a963aa..5ce711b9b070 100644
>> --- a/drivers/acpi/acpi_pnp.c
>> +++ b/drivers/acpi/acpi_pnp.c
>> @@ -319,6 +319,10 @@ static bool matching_id(const char *idstr, const char *list_id)
>>   {
>>   	int i;
>>   
>> +	/* a pnp device id has CCCdddd format (C character, d digit), strlen should be 7 */
>> +	if (strlen(idstr) != 7)
>> +		return false;
> Shouldn't you verify that the format is correct as well?

I thought the rest code in this function will verify the format, just 
missing the length checking. But I was wrong, "a pnp device id has 
CCCdddd format" is not correct since WACFXXX is a valid id, I will 
follow Rafael's advice:  compare two string's length.

Thanks.


>
> thanks,
>
> greg k-h

Re: [PATCH v2] ACPI / PNP: check the string length of pnp device id in matching_id

[Hui Wang]

On 12/10/20 9:57 PM, Rafael J. Wysocki wrote:
> On Thu, Dec 10, 2020 at 2:27 AM Hui Wang <hui.wang@canonical.com> wrote:
>> Recently we met a touchscreen problem on some Thinkpad machines, the
>> touchscreen driver (i2c-hid) is not loaded and the touchscreen can't
>> work.
>>
>> An i2c ACPI device with the name WACF2200 is defined in the BIOS, with
>> the current ACPI PNP matching rule, this device will be regarded as
>> a PNP device since there is WACFXXX in the acpi_pnp_device_ids[] and
>> this PNP device is attached to the acpi device as the 1st
>> physical_node, this will make the i2c bus match fail when i2c bus
>> calls acpi_companion_match() to match the acpi_id_table in the i2c-hid
>> driver.
>>
>> An ACPI PNP device's id has fixed format and its string length equals
>> 7, after adding this check in the matching_id, the touchscreen could
>> work.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>> ---
>>   drivers/acpi/acpi_pnp.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/acpi/acpi_pnp.c b/drivers/acpi/acpi_pnp.c
>> index 4ed755a963aa..5ce711b9b070 100644
>> --- a/drivers/acpi/acpi_pnp.c
>> +++ b/drivers/acpi/acpi_pnp.c
>> @@ -319,6 +319,10 @@ static bool matching_id(const char *idstr, const char *list_id)
>>   {
>>          int i;
>>
>> +       /* a pnp device id has CCCdddd format (C character, d digit), strlen should be 7 */
>> +       if (strlen(idstr) != 7)
>> +               return false;
> What I meant was comparing the length of the arg strings and return
> false if they don't match.  Wouldn't that work?

Oh, got it.

Thanks.

>
>> +
>>          if (memcmp(idstr, list_id, 3))
>>                  return false;
>>
>> --
>> 2.25.1
>>

Re: [PATCH v2] ACPI / PNP: check the string length of pnp device id in matching_id

[Rafael J. Wysocki]

On Fri, Dec 11, 2020 at 1:43 AM Hui Wang <hui.wang@canonical.com> wrote:
>
> On 12/10/20 5:04 PM, Greg KH wrote:
> > On Thu, Dec 10, 2020 at 09:25:39AM +0800, Hui Wang wrote:
> >> Recently we met a touchscreen problem on some Thinkpad machines, the
> >> touchscreen driver (i2c-hid) is not loaded and the touchscreen can't
> >> work.
> >>
> >> An i2c ACPI device with the name WACF2200 is defined in the BIOS, with
> >> the current ACPI PNP matching rule, this device will be regarded as
> >> a PNP device since there is WACFXXX in the acpi_pnp_device_ids[] and
> >> this PNP device is attached to the acpi device as the 1st
> >> physical_node, this will make the i2c bus match fail when i2c bus
> >> calls acpi_companion_match() to match the acpi_id_table in the i2c-hid
> >> driver.
> >>
> >> An ACPI PNP device's id has fixed format and its string length equals
> >> 7, after adding this check in the matching_id, the touchscreen could
> >> work.
> >>
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> >> ---
> >>   drivers/acpi/acpi_pnp.c | 4 ++++
> >>   1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/acpi/acpi_pnp.c b/drivers/acpi/acpi_pnp.c
> >> index 4ed755a963aa..5ce711b9b070 100644
> >> --- a/drivers/acpi/acpi_pnp.c
> >> +++ b/drivers/acpi/acpi_pnp.c
> >> @@ -319,6 +319,10 @@ static bool matching_id(const char *idstr, const char *list_id)
> >>   {
> >>      int i;
> >>
> >> +    /* a pnp device id has CCCdddd format (C character, d digit), strlen should be 7 */
> >> +    if (strlen(idstr) != 7)
> >> +            return false;
> > Shouldn't you verify that the format is correct as well?
>
> I thought the rest code in this function will verify the format, just
> missing the length checking. But I was wrong, "a pnp device id has
> CCCdddd format" is not correct since WACFXXX is a valid id,

In fact, the "F" may be regarded as a hex digit and so this follows
the CCCdddd format too.

The problem with the current code is that it matches ACPI device IDs
in the WACFdddd format against the WACFXXX string, which is incorrect.

> I will follow Rafael's advice:  compare two string's length.

Please do.